Report AdditionalLocations for CS8618 (uninitialized non-nullable member)#58096
Conversation
|
|
||
| var property = comp.GetTypeByMetadataName("C").GetMember("S"); | ||
| var actualAdditionalLocations = comp.GetDiagnostics().Single().AdditionalLocations; | ||
| Assert.Equal(property.Locations.Single(), actualAdditionalLocations.Single()); |
There was a problem hiding this comment.
@Youssef1313 not sure the additional location makes sense when the diagnostic is issued on the property (i.e. no constructor), but I'm not sure what the conventions are here.
There was a problem hiding this comment.
I'll let someone from the compiler team weigh on how this should behave. My intention was to allow the use of AdditionalLocations in all cases.
There was a problem hiding this comment.
I think not having to think about whether to use AdditionalLocations or not is the correct move.
|
Closing and re-opening for a new build |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 1). @dotnet/roslyn-compiler for a second review.
|
@jcouv @RikkiGibson Can I get a second review here? Thanks! |
| comp.VerifyDiagnostics( | ||
| // (4,12): warning CS8618: Non-nullable property 'S' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. | ||
| // public C() { } | ||
| Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C").WithArguments("property", "S").WithLocation(4, 12)); |
There was a problem hiding this comment.
It's slightly unfortunate that this generated baseline doesn't include the additional locations.
|
Thanks for the contribution @Youssef1313! |
|
@RikkiGibson and others, I'm testing with dotnet SDK 7.0.100-preview.3.22179.4 and the AdditionalLocations property on the diagnostic is empty - can you confirm this came out with preview3? (it's always a bit difficult to understand which SDK version the VS version maps to) |
|
This change should definitely be in preview3. What's the scenario/environment where you observe that AdditionalLocations is empty? |
|
Apologies, I can no longer reproduce the issue and am seeing the populated AdditionalLocations (possibly a caching issue after upgrading or something). Thanks for the quick response. |
|
@RikkiGibson sorry, one last question... If I'm understanding everything correctly, I can see AdditionalLocations populated when referencing Microsoft.CodeAnalysis.CSharp 4.2.0-2.final, but not when referencing 4.1.0. Is 4.2.0-2.final the equivalent of the Roslyn that's included in SDK 7.0.0-preview.3, and will 4.2.0 be released at the same time as .NET 7.0? |
It looks like preview3 has some build of 4.2.0-4. The way I answer these questions is by querying PRs to dotnet/sdk.
4.2.0 will be released sooner than .NET 7.0, I think. See the (internal) dev17.2 schedule and NET 7 schedule. |
|
Thanks again @RikkiGibson. |
Newer Roslyn versions emit AdditionalLocations on CS8618 diagnostics (dotnet/roslyn#58096), which causes the test framework to fail when the expected DiagnosticResult doesn't account for them. Add DiagnosticOptions.IgnoreAdditionalLocations to all suppressed diagnostic expectations so tests pass on both old and new Roslyn. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…203) * xunit/xunit#3423: Suppress CS8618 for members set in InitializeAsync Add a DiagnosticSuppressor that suppresses CS8618 (non-nullable field/property not initialized) when the member is directly assigned in an IAsyncLifetime.InitializeAsync implementation. Handles both fields and properties, including this-qualified access. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Handle constructor-targeted CS8618 and add V3 test coverage When a class has an explicit constructor, CS8618 fires on the constructor rather than the field/property. The suppressor now resolves the target member by checking AdditionalLocations and falling back to parsing the member name from the diagnostic message. Also adds V3 (ValueTask-based IAsyncLifetime) test coverage for all scenarios, bringing total tests from 6 to 16. Inspired by edge cases identified by @ssaporito in #3423. * Fix suppression tests for Roslyn 4.14+ AdditionalLocations on CS8618 Newer Roslyn versions emit AdditionalLocations on CS8618 diagnostics (dotnet/roslyn#58096), which causes the test framework to fail when the expected DiagnosticResult doesn't account for them. Add DiagnosticOptions.IgnoreAdditionalLocations to all suppressed diagnostic expectations so tests pass on both old and new Roslyn. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes #58073