Fixes a small threading issue in code generation and a TODO #2233
Conversation
|
@swift-ci please test |
| @@ -154,6 +154,7 @@ struct GenerateSwiftSyntax: ParsableCommand { | |||
| } | |||
| ) | |||
|
|
|||
| let errorsLock = NSLock() | |||
There was a problem hiding this comment.
Seems like we should use async/await with multiple tasks that we reduce after here, but that's a larger change. For this IMO just rename previouslyGenerateFilesLock and use it for both.
There was a problem hiding this comment.
Agreed that that should be the long term direction. Is this allowed in this project? I don't think that change should take long to make, provided the CI can verify that it works on Linux and windows.
There was a problem hiding this comment.
We require being able to compile with >= 5.7 currently and this is just a generation tool, so unless there's some bugs preventing it, I don't see any reason it would be an issue.
There was a problem hiding this comment.
Here is an Structured Concurrency version of this pr. If this looks good to you I can abandon this pr and pr the Structured Concurrency version instead.
There was a problem hiding this comment.
LGTM, happy for you to put that up instead. Regarding the grouping below, I wouldn't worry about indenting personally - this is just for generation which typically shouldn't fail. What you could do is just separate them by file path, eg. ```
Errors occurred during generation.
'<file>':
<error here>
'<file>':
<error here>
There was a problem hiding this comment.
Created #2245. It also incorporates this feedback about printing which file produced which error.
There was a problem hiding this comment.
Nice, using structured concurrency is so much cleaner. Thanks for putting it up. This means we can close this PR, right?
| // TODO: It would be nice if we could emit all errors | ||
| throw firstError | ||
| if errors.count > 0 { | ||
| struct ComposititeError: Error, CustomStringConvertible { |
There was a problem hiding this comment.
| struct ComposititeError: Error, CustomStringConvertible { | |
| struct CompositeError: Error, CustomStringConvertible { |
Personally I'd define this top level, probably at the bottom of this file.
| struct ComposititeError: Error, CustomStringConvertible { | ||
| var errors: [Error] | ||
| var description: String { | ||
| "Multiple errors emitted: \n" + |
There was a problem hiding this comment.
| "Multiple errors emitted: \n" + | |
| "Multiple errors emitted:\n" + |
| "Multiple errors emitted: \n" + | ||
| errors | ||
| .map(String.init(describing: )) | ||
| .joined(separator: "\n") |
There was a problem hiding this comment.
Do they already have a newline, or would it be worth adding an extra newline here? Do you happen to have an example of the output here for multiple errors?
There was a problem hiding this comment.
I can make you some real output in a few hours, but in my original testing NSError does not include a newline, and that's the error that we're most likely to encounter in this code path today due to the use of FileManager.
I think in general that it will depend on how the error implements its description or debugDescription var and that the safest option is to add a newline even if we can find some errors that happen to do this for us.
There was a problem hiding this comment.
Sorry, to be clear, we definitely want at least one newline here. My question was about whether we might want two so that there's a clearer cut between the errors.
There was a problem hiding this comment.
Ah, yeah that makes a lot more sense. Here's the sample output of an NSError in this context:
Error: Multiple errors emitted:
Error Domain=test error Code=0 "(null)"
Error Domain=test error Code=0 "(null)"
Error Domain=test error Code=0 "(null)"
Error Domain=test error Code=0 "(null)"
Error Domain=test error Code=0 "(null)"
Error Domain=test error Code=0 "(null)"
I think in an ideal world it would look something like
Multiple errors emitted: {
Error Domain=test error Code=0 "(null)"
Error Domain=test error Code=0 "(null)"
...
}
because they're separate, but grouped. But that would require adding a means of indenting text (in case of multi-line errors) which might be overkill. But I'm happy to add the indentation and newline, or just the second newline, or leave it out, whichever you prefer.
|
Closing because it’s been superseded by #2245 AFAICT. |
errorswas potentially mutated on multiple threads.previouslyGeneratedFiles, which is mutated on every loop, is locked just above it, so clearly we need to lock on all state that's mutated here.I also cleaned up the TODO.