Fix warnings for DAM.All #2191
Fix warnings for DAM.All #2191
Conversation
- Remove unnecessary helper
| case TypeDefinition nestedType: | ||
| DependencyInfo nestedDependencyInfo = new DependencyInfo (dependencyKind, reflectionContext.Source); | ||
| reflectionContext.RecordRecognizedPattern (nestedType, () => _markStep.MarkEntireType (nestedType, includeBaseAndInterfaceTypes: true, nestedDependencyInfo)); | ||
| MarkType (ref reflectionContext, nestedType, dependencyKind); |
There was a problem hiding this comment.
I must admit I'm a bit nervous about potential endless recursion. After all that's why MarkEntireType has the hashset - to avoid that. This is basically trying to do the same thing - without it.
If I remember correctly the counter example for MarkEntireType was something like this:
class MyAttribute : Attribute
{
MyAttribute([DAM(All)] Type type) {}
}
[MyAttribute(typeof(TestType))]
class TestType
{
}This would be endless since MarkEntireType calls MarkCustomAttributes which will in turn eventually call MarkEntireType (due to the All annotation) and so on.
I think this new code will work, since MarkType effectively has a hashset in the CheckProcessed which happens before the custom attribute marking. (Similar repro can be made via base types or implemented interfaces with generics - but the solution is the same I hope).
So I guess there's no problem in the end, I hope...
There was a problem hiding this comment.
Can you add a test case along these lines. The base type one would look like:
class Base<[DAM(All)] T> {}
class TestType : Base<TestType> {}Similarly for interfaces.
Another one is attributes on members: We already have an issue with this on properties and other members - see #2196 (I just created this). The All is basically just another case of the same problem for these members.
There was a problem hiding this comment.
Yup, the first example is already covered by this test: https://github.com/mono/linker/blob/e21bb44ca909d33c18acf3ed0b68f94a413351a5/test/Mono.Linker.Tests.Cases/DataFlow/AttributeConstructorDataflow.cs#L96-L100.
I added the second testcase you suggested - I think generic type parameters shouldn't be an issue since the DAM binder only gives back members and doesn't look at the instantiated base generic type at all (instead it relies on normal MarkType logic to mark generic arguments).
Good find about attributes on properties/events.
There was a problem hiding this comment.
(instead it relies on normal MarkType logic to mark generic arguments).
That was the point - I don't think MarkType was written with reentrancy in mind - it seems to handle it, which is good, but I thought it would be good to have tests for it. Before data flow, there were only a few ways to get to the reentrancy, now there are many more.
But I'm fine if we try to cover this later on.
There was a problem hiding this comment.
Yeah, it looks like it was at least given some thought but it's good to have more tests as we add more ways to get to MarkType. I did add the generic tests you suggested above so I think that is covered.
| using var _ = _scopeStack.CurrentScope.Origin.MemberDefinition == null ? _scopeStack.PushScope (new MessageOrigin (type)) : null; | ||
|
|
||
| bool isDynamicDependencyReason = reason.Kind == DependencyKind.DynamicallyAccessedMember || reason.Kind == DependencyKind.DynamicDependency || reason.Kind == DependencyKind.DynamicallyAccessedMemberOnType; | ||
| if (!_entireTypesMarked.Add (type)) |
There was a problem hiding this comment.
Why not use the same mechanism. I know there are slight behavioral differences (around interfaces and such), but still. We now have two completely different implementations of basically the same problem. We could get rid of the _entireTypesMarked completely and solely rely on CheckProcessed.
- Resolve -> TryResolve - Avoid nested yield returns - Testcases with base instantiated over self for generic parameter with requirements
| // For now, annotations that are inherited from base types but also declared explicitly on this type | ||
| // still warn about base members since the cache doesn't track enough information to tell the difference. |
There was a problem hiding this comment.
I think this is fine - the only case where I can think of this making sense is if the derived type adds more annotations (base has all methods and derived type has both methods and fields) - but that is SO unlikely and rare - I don't care :-).
vitek-karas
left a comment
vitek-karas
left a comment
There was a problem hiding this comment.
Looks good.
I would like us to de-dupe the code between data flow and MarkEntireType, but it can be done in a followup change.
|
I almost finished deduping those pieces of code yesterday but hit some test failures I didn't understand. I'll do it in a follow-up. |
dotnet#2191 addressed issues with redundant warnings from base types, but interfaces need to be handled specially. There were a few issues with the interface handling in that change: - The binding logic didn't include interfaces for DAM.All when declaredOnly was specified, as @vitek-karas pointed out in dotnet#2206 (comment). This could have led to us not marking interfaces for types derived from a DAMT.All-annotated type. Either way, the binding logic should not select interface _members_ when declaredOnly is specified. - The DAMT-on-type logic had special handling for base types, but didn't separately mark interface members, which is needed when applying annotations DAMT.All or DAMT.Interfaces. This could have led to missing interface impls or interface members for types which have (explicit or inherited) DAMT.All or DAMT.Interfaces annotations. Separately, the bit operations on DAMT also didn't correctly handle cases where there are bits shared by different enum members.
* Fix warnings for DAMT on interfaces #2191 addressed issues with redundant warnings from base types, but interfaces need to be handled specially. There were a few issues with the interface handling in that change: - The binding logic didn't include interfaces for DAM.All when declaredOnly was specified, as @vitek-karas pointed out in #2206 (comment). This could have led to us not marking interfaces for types derived from a DAMT.All-annotated type. Either way, the binding logic should not select interface _members_ when declaredOnly is specified. - The DAMT-on-type logic had special handling for base types, but didn't separately mark interface members, which is needed when applying annotations DAMT.All or DAMT.Interfaces. This could have led to missing interface impls or interface members for types which have (explicit or inherited) DAMT.All or DAMT.Interfaces annotations. Separately, the bit operations on DAMT also didn't correctly handle cases where there are bits shared by different enum members. * Remove debug output * Apply suggestions from code review Co-authored-by: Vitek Karas <[email protected]> Co-authored-by: Vitek Karas <[email protected]>
* Fix warnings for DAM.All Fixes dotnet/linker#2159 * Add tests * PR feedback - Remove unnecessary helper * Avoid redundant DAM warnings for base members * PR feedback - Resolve -> TryResolve - Avoid nested yield returns - Testcases with base instantiated over self for generic parameter with requirements Commit migrated from dotnet/linker@d8b94bd
* Fix warnings for DAMT on interfaces dotnet/linker#2191 addressed issues with redundant warnings from base types, but interfaces need to be handled specially. There were a few issues with the interface handling in that change: - The binding logic didn't include interfaces for DAM.All when declaredOnly was specified, as @vitek-karas pointed out in dotnet/linker#2206 (comment). This could have led to us not marking interfaces for types derived from a DAMT.All-annotated type. Either way, the binding logic should not select interface _members_ when declaredOnly is specified. - The DAMT-on-type logic had special handling for base types, but didn't separately mark interface members, which is needed when applying annotations DAMT.All or DAMT.Interfaces. This could have led to missing interface impls or interface members for types which have (explicit or inherited) DAMT.All or DAMT.Interfaces annotations. Separately, the bit operations on DAMT also didn't correctly handle cases where there are bits shared by different enum members. * Remove debug output * Apply suggestions from code review Co-authored-by: Vitek Karas <[email protected]> Co-authored-by: Vitek Karas <[email protected]> Commit migrated from dotnet/linker@c321df2
4 participants
Fixes #2159.
This should also fix #2160 but I'm not sure we have a great way to test that. I will look into it more later today.