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

Report AdditionalLocations for CS8618 (uninitialized non-nullable member) #58096

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 3, 2021

Fixes #58073

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 3, 2021 08:52
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 3, 2021

var property = comp.GetTypeByMetadataName("C").GetMember("S");
var actualAdditionalLocations = comp.GetDiagnostics().Single().AdditionalLocations;
Assert.Equal(property.Locations.Single(), actualAdditionalLocations.Single());
Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think not having to think about whether to use AdditionalLocations or not is the correct move.

@Youssef1313
Copy link
Member Author

Closing and re-opening for a new build

@Youssef1313 Youssef1313 closed this Jan 4, 2022
@Youssef1313 Youssef1313 reopened this Jan 4, 2022
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1). @dotnet/roslyn-compiler for a second review.

@Youssef1313
Copy link
Member Author

@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));
Copy link
Contributor

Choose a reason for hiding this comment

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

It's slightly unfortunate that this generated baseline doesn't include the additional locations.

@RikkiGibson RikkiGibson merged commit 67c038a into dotnet:main Jan 28, 2022
@ghost ghost added this to the Next milestone Jan 28, 2022
@RikkiGibson
Copy link
Contributor

Thanks for the contribution @Youssef1313!

@Youssef1313 Youssef1313 deleted the cs8618-additionallocations branch January 28, 2022 19:47
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
@roji
Copy link
Member

roji commented Apr 22, 2022

@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)

@RikkiGibson
Copy link
Contributor

This change should definitely be in preview3. What's the scenario/environment where you observe that AdditionalLocations is empty?

@roji
Copy link
Member

roji commented Apr 22, 2022

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.

@roji
Copy link
Member

roji commented Apr 22, 2022

@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?

@RikkiGibson
Copy link
Contributor

Is 4.2.0-2.final the equivalent of the Roslyn that's included in SDK 7.0.0-preview.3

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.

will 4.2.0 be released at the same time as .NET 7.0?

4.2.0 will be released sooner than .NET 7.0, I think. See the (internal) dev17.2 schedule and NET 7 schedule.

@roji
Copy link
Member

roji commented Apr 26, 2022

Thanks again @RikkiGibson.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AdditionalLocations for CS8618 in DiagnosticSuppressor
4 participants