Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

Fixes #6573

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner June 7, 2023 23:07
@CyrusNajmabadi
Copy link
Member Author

@sharwell @mavasani ptal.

@CyrusNajmabadi
Copy link
Member Author

@mavasani @sharwell , updating to the new roslyn is producing errors like:

Code fix is attempting to provide a fix for a non-local analyzer diagnostic
Expected: False
Actual:   True

It looks like we need to update analyzers here to follow the new rules you set out that diagnostics can only be reported within the span of hte entity we're getting a a callback for.

@mavasani
Copy link

mavasani commented Jun 8, 2023

@mavasani @sharwell , updating to the new roslyn is producing errors like:

Code fix is attempting to provide a fix for a non-local analyzer diagnostic
Expected: False
Actual:   True

It looks like we need to update analyzers here to follow the new rules you set out that diagnostics can only be reported within the span of hte entity we're getting a a callback for.

Yes, this is a pending work item, see #6560 (comment). However, I don't know if any of the existing tests for this analyzer failed with this issue - @sharwell might know more.

You can disable the non-local diagnostic check with CodeFixTestBehaviors = CodeFixTestBehaviors.SkipLocalDiagnosticCheck just like in the comment above. Fix for #6598 should remove all the usages of CodeFixTestBehaviors.SkipLocalDiagnosticCheck

@mavasani
Copy link

mavasani commented Jun 8, 2023

Tagging @buyaa-n

@CyrusNajmabadi - do we need to backport this fix to .NET 7 servicing branch? I am not sure if primary constructors are available with compiler shipping in .NET 7 SDK.

@CyrusNajmabadi
Copy link
Member Author

Just to be clear. The "non-local" errors are from other tests. You would still like me to dsiable them?

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Jun 8, 2023

Importantly, it looks like there are several hundred tests in this category:

image

@mavasani
Copy link

mavasani commented Jun 8, 2023

Importantly, it looks like there are several hundred tests in this category:

image

Oh that is a huge number. Probably coming from further updates to non-local diagnostics semantics. If your PR is okay to wait till early next week, I can work on this today/tomorrow/over the weekend. If not, feel free to disable non-local diagnostic check for all the failures and merge your PR, I'll try to get to them as soon as possible.

@CyrusNajmabadi
Copy link
Member Author

Sounds good.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 8, 2023 02:53
@CyrusNajmabadi CyrusNajmabadi merged commit 2f6e15a into main Jun 8, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the primaryConstructors branch June 8, 2023 03:14
@github-actions github-actions bot added this to the vNext milestone Jun 8, 2023
@CyrusNajmabadi
Copy link
Member Author

@mavasani can you help update dotnet/roslyn to use this new version of roslyn-analyzers? Thanks!

@cfbao
Copy link

cfbao commented Jun 20, 2023

Will this be included in the next .NET 8 preview release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Primary constructor parameter gives incorrect "could be static" warning

4 participants