-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not access Workspace.CurrentSolution when cleaning up a code-action's operations. #67116
Conversation
Touching the virtual extensibility points is terrifying. I'd prefer to definitely not do that in this PR. :) |
@@ -16,6 +16,7 @@ | |||
|
|||
namespace Microsoft.CodeAnalysis.CodeActions | |||
{ | |||
#pragma warning disable RS0030 // Do not used banned APIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 We should probably fix RS0030 to not report diagnostics in comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd disagree here -- this is correctly pointing to a API smell. We have a documentation comment telling people to use APIs that we know are broken, but aren't providing a proper replacement for.
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.targets', '$(MSBuildThisFileDirectory)../'))" /> | ||
|
||
<ItemGroup> | ||
<AdditionalFiles Include="$(MSBuildThisFileDirectory)\BannedSymbols.txt" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Why not just add it to one of our existing files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we want to ban this symbol in all of our projects, not just one of them. Place it here:
https://github.com/dotnet/roslyn/blob/main/eng/config/BannedSymbols.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh. why is that file not in our vs solution. moving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
@sharwell ptal. |
@@ -135,25 +136,25 @@ internal Guid GetTelemetryId(FixAllScope? fixAllScope = null) | |||
/// The sequence of operations that define the code action. | |||
/// </summary> | |||
public Task<ImmutableArray<CodeActionOperation>> GetOperationsAsync(CancellationToken cancellationToken) | |||
=> GetOperationsAsync(new ProgressTracker(), cancellationToken); | |||
=> GetOperationsAsync(originalSolution: null!, new ProgressTracker(), cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the use of the ! here -- given the methods being called are internal, can we at least annotate the internal methods correctly? I get this is a back-compat path only, but it seems those rare "gotchas" are exactly why a null annotation might be useful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also concerned that even if the banned API more or less means we can't hit this path in our product, we still have a vulnerability for other hosts I think. Should we at least have a few tests testing the old path to ensure we don't break it if we're not using NRT to ensure safety?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the methods being called are internal, can we at least annotate the internal methods correctly?
Because i don't want internal calls to ever pass null. If we annotate as 'null' that can happen. Annotating as non-null means we catch our own mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option is [DisallowNull] Solution? originalSolution
, but I'm not sure it's better.
@@ -135,25 +136,25 @@ internal Guid GetTelemetryId(FixAllScope? fixAllScope = null) | |||
/// The sequence of operations that define the code action. | |||
/// </summary> | |||
public Task<ImmutableArray<CodeActionOperation>> GetOperationsAsync(CancellationToken cancellationToken) | |||
=> GetOperationsAsync(new ProgressTracker(), cancellationToken); | |||
=> GetOperationsAsync(originalSolution: null!, new ProgressTracker(), cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be making this method that takes the original solution public? Otherwise any custom implementations still will have bugs, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can consider that. generally speaking though that's not been what we've done with anything else. We'd also potentially want to really rethink these apis overall (they're terrible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
want to really rethink these apis overall (they're terrible).
+1 on that. I don't think we should be introducing a new public API that address only this issue. The CodeAction APIs have multiple other issues as well that need new API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise any custom implementations still will have bugs, right?
Depends on the workspace and the type of code action. Code actions that just change code won't have problems. Code actions in workspaces that aren't updated in parallel with the code action application also aren't affected. Past that the scenarios are rare enough that I think we can defer.
@@ -16,6 +16,7 @@ | |||
|
|||
namespace Microsoft.CodeAnalysis.CodeActions | |||
{ | |||
#pragma warning disable RS0030 // Do not used banned APIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd disagree here -- this is correctly pointing to a API smell. We have a documentation comment telling people to use APIs that we know are broken, but aren't providing a proper replacement for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up that this PR will be going into 17.6-preview3, not preview2. This might be fine with you, just wanted to make sure you knew. |
Fine with me @RikkiGibson |
Fixes #42779