Fix crash when compiling internal attribute constructor #42192
Conversation
| foreach (var method in this.GetMethodsToEmit()) | ||
| { | ||
| Debug.Assert((object)method != null); | ||
| if (method.ShouldInclude(context)) | ||
| if (isAttribute || method.ShouldInclude(context)) |
There was a problem hiding this comment.
isAttribute [](start = 20, length = 11)
It feels unfortunate that we are going to include all attribute constructors unconditionally. Also, I am curious what other scenarios we are missing when a declaration that isn't included by default is referenced by some other API that is included. However, building a NoPia-like tracking feels like an overkill just to get the attributes case working.
| foreach (var method in this.GetMethodsToEmit()) | ||
| { | ||
| Debug.Assert((object)method != null); | ||
| if (method.ShouldInclude(context)) | ||
| if (isAttribute || method.ShouldInclude(context)) |
There was a problem hiding this comment.
isAttribute [](start = 20, length = 11)
isAttribute [](start = 20, length = 11)
It looks like we are including all methods, not just instance constructors #Closed
| using System; | ||
| internal class SomeAttribute : Attribute | ||
| { | ||
| internal SomeAttribute() |
There was a problem hiding this comment.
internal SomeAttribute() [](start = 4, length = 24)
Please add a regular internal method and verify that it is not emitted. #Closed
|
Done with review pass (iteration 1) #Closed |
| @@ -661,10 +661,13 @@ IEnumerable<Cci.IMethodDefinition> Cci.ITypeDefinition.GetMethods(EmitContext co | |||
| { | |||
| CheckDefinitionInvariant(); | |||
|
|
|||
| // All constructors in attributes should be emitted | |||
| bool isAttribute = DeclaringCompilation.IsAttributeType(this); | |||
There was a problem hiding this comment.
bool isAttribute = DeclaringCompilation.IsAttributeType(this); [](start = 11, length = 63)
We probably should avoid calculating this when nothing is going to be filtered out anyway. #Closed
There was a problem hiding this comment.
What's the best code pattern to do something like this? I feel like Lazy with a lambda is too heavyweight, and a bool hasComputedIsAttribute; bool isAttribute; is kind of gross (so I want confirmation before doing that). (Or did you mean something else by that? Not computing if context.IncludePrivateMembers is true or something like that?)
There was a problem hiding this comment.
Or did you mean something else by that? Not computing if context.IncludePrivateMembers is true or something like that?)
Yes, something like that. Do not calculate if ShouldInclude doesn't filter anything out.
In reply to: 388878100 [](ancestors = 388878100)
There was a problem hiding this comment.
I would also rename the local to something like alwaysIncludeConstructors it will cause less confusion once its value is conditionally calculated. The type doesn't stop to be an attribute no matter what ShouldInclude does.
In reply to: 389016262 [](ancestors = 389016262,388878100)
khyperia
force-pushed
the
internal-attribute-refout
branch
from
6a02ba9 to
0a812dc
Compare
|
@khyperia Please do not rewrite commits that undergo review process. Do not force push into the PR branches until review process is complete. This prevents people from doing incremental review and often messes up comment tracking. Simply make necessary changes in new commits and push. Commits can easily be squashed on merge, or you can rebase right before merging. Thanks. #Closed |
| @@ -676,7 +679,7 @@ IEnumerable<Cci.IMethodDefinition> Cci.ITypeDefinition.GetMethods(EmitContext co | |||
| { | |||
| foreach (var m in generated) | |||
| { | |||
| if (m.ShouldInclude(context)) | |||
| if ((isAttribute && m.IsConstructor) || m.ShouldInclude(context)) | |||
There was a problem hiding this comment.
IsConstructor [](start = 42, length = 13)
Can we use the same way to perform the constructor check here and above? #Closed
There was a problem hiding this comment.
Surprisingly, no - in the "normal" (not generated) method case, method is a MethodSymbol, but in the generated method case, m is a Cci.IMethodDefinition. It doesn't seem like there's a common API between them to check if it's a constructor.
Sorry about that, different projects have different github practices, I forgot what Roslyn used 😢 |
| { | ||
| } | ||
|
|
||
| internal void F() |
There was a problem hiding this comment.
internal void F() [](start = 4, length = 17)
Please also add a test with a static constructor. #Closed
|
@jcouv, @dotnet/roslyn-compiler For the second sign-off. |
|
@jcouv Could you please take a look at this PR. Are you Ok with the impact for the feature? |
jaredpar
added
the
Community
|
Sorry, I'd missed the ping. I'll take a look today. |
Yes, it's okay. @khyperia Thanks for the fix. Could you also update the refout doc as part of this PR to document the additional rule? https://github.com/dotnet/roslyn/blob/master/docs/features/refout.md |
|
@khyperia Once you update the doc, would you mind if we squash commits during the merge? |
|
@AlekseyTs Please do! |
|
@jcouv If changes to the doc are fine, could you please merge this PR using "Squash and merge" mode? |
ghost
added this to the
|
Thanks @khyperia! I hope you're doing well :-) |
* Fix crash when compiling internal attribute constructor * PR feedback * Add new rule to refout.md
Fixes #38444
Updated version of #38873, with a different fix (that I think is more correct - this code pattern matches the one for struct fields). Still though, I'm not 100% sure this is the right fix, feedback would be appreciated if not!