Skip to content
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

Fix for FixAll in project and solution scopes for non-C#/VB projects #67650

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

mavasani
Copy link
Member

@mavasani mavasani commented Apr 5, 2023

F# team identified a bug in FixAll support where in FixAll works fine for Document scope, but doesn't work for Project and Solution scope. The core issue is in FixAllContextHelper code path for these scopes, where we reverse map from diagnostic location to source document. Existing code was assuming the diagnostic location has an associated source syntax tree, which is only true for C# and VB. We now handle diagnostics with external locations that map back to source documents in non-C#/VB world.

I manually verified that this fixes the F# FixAll scenario. I spent some time investigating writing an F# FixAll unit test, but that seems to have lot of missing pieces. Instead, I have requested F# team to add a FixAll unit test or integration test in their repo once this fix flows to them.

F# team identified a bug in FixAll support where in FixAll works fine for Document scope, but doesn't work for Project and Solution scope. The core issue is in FixAllContextHelper code path for these scopes, where we reverse map from diagnostic location to source document. Existing code was assuming the diagnostic location has an associated source syntax tree, which is only true for C# and VB. We now handle diagnostics with external locations that map back to source documents in non-C#/VB world.

I manually verified that this fixes the F# FixAll scenario. I spent some time investigating writing an F# FixAll unit test, but that seems to have lot of missing pieces. Instead, I have requested F# team to add a FixAll unit test or integration test in their repo once this fix flows to them.
@mavasani mavasani requested a review from a team as a code owner April 5, 2023 13:21
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 5, 2023
@mavasani mavasani requested a review from T-Gro April 5, 2023 13:22
@psfinaki
Copy link
Member

psfinaki commented Apr 5, 2023

More context: here and here.
As for tests, yes, I plan adding some proper codefix testing framework in F# repo.

Thanks!

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test would be good. but not blocking. note: we can make compilations that simulate having compilations from other languages (we do this to add tests when we break TS for example) :)

@mavasani
Copy link
Member Author

mavasani commented Apr 5, 2023

test would be good. but not blocking. note: we can make compilations that simulate having compilations from other languages (we do this to add tests when we break TS for example) :)

Good idea. I’ll try to add one with a follow up PR

@mavasani mavasani enabled auto-merge April 5, 2023 15:36
@mavasani mavasani merged commit 5ffbba9 into dotnet:main Apr 5, 2023
25 checks passed
@ghost ghost added this to the Next milestone Apr 5, 2023
@dibarbet dibarbet modified the milestones: Next, 17.7 P1 Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants