Pass cancellation token consistently through lsif generator.#65875
Pass cancellation token consistently through lsif generator.#65875CyrusNajmabadi merged 10 commits intodotnet:mainfrom
Conversation
ghost
added
the
| public async Task GenerateForProjectAsync( | ||
| Project project, | ||
| GeneratorOptions options, | ||
| CancellationToken cancellationToken) |
There was a problem hiding this comment.
So we generally weren't worrying about cancellation at all since we don't have any way to produce a cancellation token from the start -- did you have something in mind where we might have one?
There was a problem hiding this comment.
Yup. A totally reasonable pattern is to register to hear about ctrl-c and prompt, or use that for cancellation. I agree this it not a big deal here as we're ok with just killing things. But i like code with good hygiene :)
|
@jasonmalinowski ptal. |
| if (document is SourceGeneratedDocument) | ||
| { | ||
| var text = await document.GetTextAsync().ConfigureAwait(false); | ||
| var text = await document.GetTextAsync(); |
There was a problem hiding this comment.
💭 I thought GetTextAsync would have accepted a cancellation token
| _ => throw new NotImplementedException() | ||
| }; | ||
|
|
||
| var cancellationToken = CancellationToken.None; |
| @@ -81,21 +90,21 @@ private static async Task GenerateAsync(FileInfo? solution, FileInfo? project, F | |||
| if (solution != null) | |||
| { | |||
| await LocateAndRegisterMSBuild(logFile); | |||
There was a problem hiding this comment.
❔ Missing cancellation token?
There was a problem hiding this comment.
nothing in that method needs CT as fat as i can tell :)
| Assert.Empty(compilation.GetDiagnostics().Where(Function(d) d.Severity = DiagnosticSeverity.Error)) | ||
|
|
||
| Await lsifGenerator.GenerateForProjectAsync(project, GeneratorOptions.Default) | ||
| Await lsifGenerator.GenerateForProjectAsync(project, GeneratorOptions.Default, CancellationToken.None) |
There was a problem hiding this comment.
for tests, i'm ok with this.
|
@jasonmalinowski ptal. |
| # CA2016: Forward the 'CancellationToken' parameter to methods | ||
| dotnet_diagnostic.CA2016.severity = warning |
There was a problem hiding this comment.
I see this already being set here:
roslyn/eng/config/globalconfigs/Shipping.globalconfig
Lines 6 to 7 in 6ff1b01
Why didn't that take effect?
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
@CyrusNajmabadi Signing off if this is really something you want to take, but I'm not sure the "hygiene" here really feels worth it to me. Since we're not actually using this token anywhere, the code that's now trying to be cancellation safe is still not being exercised or tested in any way, and now this will just mean we get to spend more time maintaining passing the token around.
So basically, i'd prefer all our code follow the same rules. That way it's trivial to move/refactor code around and not have to go "oh... this code was originally in the console, but now it's in library code, so we have to update everything it does". Having disparate islands of rules also just makes things more complicated when working as as you jump around from library to console yo have to take on disparate sets of rules. Finally, i find reviewing much more complex as these things are literally triggers. e.g. when i see CancellationToken.None i have to expend mental cycles figuring out what that means and why it's ok. When all our code follows the same rules, i just don't have to care. |
4 participants
Followup to #65858