Skip to content

Produce a specific error for 'enum' as constraint#59612

Merged
jcouv merged 7 commits intodotnet:mainfrom
jcouv:enum-constraint
Mar 1, 2022
Merged

Produce a specific error for 'enum' as constraint#59612
jcouv merged 7 commits intodotnet:mainfrom
jcouv:enum-constraint

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Feb 17, 2022

Fixes #59495

@jcouv jcouv self-assigned this Feb 17, 2022
@ghost ghost added the Area-Compilers label Feb 17, 2022
@sharwell
Copy link
Copy Markdown
Contributor

❔ Should we handle delegateSystem.Delegate since it was also added for C# 7.3?

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Feb 17, 2022

Should we handle delegate→System.Delegate since it was also added for C# 7.3?

Sure. Added

@jcouv jcouv marked this pull request as ready for review February 17, 2022 18:59
@jcouv jcouv requested a review from a team as a code owner February 17, 2022 18:59
<value>Line contains different whitespace than the closing line of the raw string literal: '{0}' versus '{1}'</value>
</data>
<data name="ERR_NoEnumConstraint" xml:space="preserve">
<value>Keyword 'enum' cannot be used as a constraint. Did you mean 'System.Enum'?</value>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<value>Keyword 'enum' cannot be used as a constraint. Did you mean 'System.Enum'?</value>
<value>Keyword 'enum' cannot be used as a constraint. Did you mean 'where T : struct, System.Enum'?</value>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I lean against this change (the original text is a better match to the variety of scenarios supported)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I genuinely disagree. The original text isn't really the recommendation (it would allow System.Enum itself, which most people do not want, and can lead to confusing scenarios since it doesn't actually contrain just to actual enums). Since we know their intent (when they write enum) i think we should suggest the best lang consturct to match that intent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with Cyrus here. It feels like the struct, System.Enum version is the intent being expressed by the user.

{
var missingType = this.AddError(this.CreateMissingIdentifierName(), ErrorCode.ERR_NoEnumConstraint);
missingType = AddTrailingSkippedSyntax(missingType, this.EatToken());
return _syntaxFactory.TypeConstraint(missingType);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i still think we should make this a struct-constraint (to help with later stuff). but it's nbd.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's tempting, but I'd rather not give too much meaning to a bad token. It could be a legitimate misspelling (from Anum?).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think that seems highly ulikely. enum is a strong c# keyword, and i personally think it's totally clear what hte user is trying to say when they add that constraint. Indeed, it's a constraint we've even considered, we just didn't do it since it falls out from where T : struct, System.Enum :)

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Feb 23, 2022

@dotnet/roslyn-compiler for review.

N(SyntaxKind.EndOfFileToken);
}
EOF();
}
Copy link
Copy Markdown
Contributor

@cston cston Feb 23, 2022

Choose a reason for hiding this comment

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

}

Consider testing the delegate case at the end of the file: record R<T> where T : delegate #Resolved

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Feb 24, 2022

@dotnet/roslyn-compiler for second review. Thanks

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Feb 28, 2022

@333fred @dotnet/roslyn-compiler for second review. Small parser error recover improvement. Thanks

Copy link
Copy Markdown
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.

Done review pass (commit 5)

<value>Line contains different whitespace than the closing line of the raw string literal: '{0}' versus '{1}'</value>
</data>
<data name="ERR_NoEnumConstraint" xml:space="preserve">
<value>Keyword 'enum' cannot be used as a constraint. Did you mean 'System.Enum'?</value>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with Cyrus here. It feels like the struct, System.Enum version is the intent being expressed by the user.

@jcouv jcouv requested a review from 333fred March 1, 2022 17:50
Copy link
Copy Markdown
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 6)

@jcouv jcouv merged commit ce1461d into dotnet:main Mar 1, 2022
@jcouv jcouv deleted the enum-constraint branch March 1, 2022 20:53
@ghost ghost added this to the Next milestone Mar 1, 2022
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
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.

Compiler should provide a better error message for generic type constraint 'enum'

6 participants