-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Warn if RequiresXXXCode is placed on an entrypoint #110185
Conversation
Fixes dotnet#109811. Motivated by real-world first party code that did this :(.
@@ -128,6 +128,11 @@ internal static MessageContainer CreateErrorMessage(MessageOrigin? origin, Diagn | |||
if (context.IsWarningSubcategorySuppressed(subcategory)) | |||
return null; | |||
|
|||
// If the warning comes from compiler-generated code, it would not be actionable. The assumption is that | |||
// compiler-generated code doesn't have issues. | |||
if (origin.MemberDefinition is MethodDesc originMethod && originMethod.GetTypicalMethodDefinition() is not EcmaMethod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ILC generates a method that calls user's main (after initializing the environment). It was triggering a warning if Main has RUC
@@ -203,5 +203,27 @@ public static bool IsConstructor ([NotNullWhen (returnValue: true)] this ISymbol | |||
|
|||
public static bool IsStaticConstructor ([NotNullWhen (returnValue: true)] this ISymbol? symbol) | |||
=> (symbol as IMethodSymbol)?.MethodKind == MethodKind.StaticConstructor; | |||
|
|||
public static bool IsEntryPoint (this IMethodSymbol methodSymbol, Compilation compilation) | |||
=> methodSymbol.Name is WellKnownMemberNames.EntryPointMethodName or WellKnownMemberNames.TopLevelStatementsEntryPointMethodName && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how Roslyn does it in same named method. It might be overly broad in edge cases. Not sure how much we care, it's pretty niche.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my only nit is that it might be nice to have separate messages for UCO and Main, but I don't feel strongly about it.
Yeah, that's a lot of extra docs and messages to write. This should be rare enough that it doesn't matter. |
Fixes #109811.
Motivated by real-world first party code that did this :(.
Cc @dotnet/illink