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.

Sign up for GitHub

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

Jump to bottom

Report better errors when multiple invalid constraints are combined #45147

Merged
merged 4 commits into from
Jun 19, 2020
Merged

Report better errors when multiple invalid constraints are combined #45147

merged 4 commits into from
Jun 19, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jun 13, 2020 •
edited
Loading

Instead of reporting that a constraint must come first, if there are multiple constraints that must come first, we now report that these constraints cannot be combined. We also suppress these errors if a previous error about inherited constraints was reported. Fixes #45141.

@333fred 333fred requested a review from a team as a code owner June 13, 2020 19:33
@333fred 333fred added Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. labels Jun 13, 2020
@333fred
Report better errors when multiple invalid constraints are combined
7ca0c7e 
Instead of reporting that a constraint must come first, if there are multiple constraints that must come first, we now report that these constraints cannot be combined. We also suppress these errors if a previous error about inherited constraints was reported. Fixes dotnet#45141.
@xparadoxical
Copy link

if there are multiple constraints that must come first, we now report that these constraints cannot be combined.

does it mean that var and nameof won't be allowed to be in the same line?

@333fred
Copy link
Member Author

333fred commented Jun 14, 2020

if there are multiple constraints that must come first, we now report that these constraints cannot be combined.

does it mean that var and nameof won't be allowed to be in the same line?

@xparadoxical sorry, I copied the wrong URL in the description. This PR is unrelated to that bug.

@333fred
Copy link
Member Author

333fred commented Jun 16, 2020

@dotnet/roslyn-compiler for reviews on this small diagnostic message cleanup

RikkiGibson
RikkiGibson approved these changes Jun 16, 2020
View reviewed changes
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM, just surprised that we didn't have a localizable way of referring to the 'class' and 'struct' language concepts until now.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 16, 2020 •
edited
Loading

Done with review pass (iteration 2), tests are not looked at #Closed

@333fred
Copy link
Member Author

333fred commented Jun 18, 2020

@AlekseyTs addressed feedback.

In reply to: 645028784 [](ancestors = 645028784)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 18, 2020 •
edited
Loading

@333fred It looks like there are legitimate test failures. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 18, 2020 •
edited
Loading

Done with review pass (iteration 3) #Closed

@333fred
Copy link
Member Author

333fred commented Jun 18, 2020

@AlekseyTs addressed feedback.

In reply to: 645772487 [](ancestors = 645772487)

AlekseyTs
AlekseyTs approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 4)

@333fred 333fred merged commit 92d1ce0 into dotnet:master Jun 19, 2020
@333fred 333fred deleted the notnull-clarity branch June 19, 2020 18:31
@ghost ghost added this to the Next milestone Jun 19, 2020
@dibarbet dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees
No one assigned
Labels
Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings.
Projects
None yet
Milestone
16.7.P4
Development

Successfully merging this pull request may close these issues.

where T : class, notnull error is confusing
6 participants
@333fred @xparadoxical @AlekseyTs @RikkiGibson @Youssef1313 @dibarbet