Fix suppression check with assembly suppressions #2180
Conversation
The change in dotnet#2171 was incorrect because it didn't account for the possibility that the suppressions cache already contains assembly or module suppressions for the provider. This fixes the check by re-using the cache to also track whether we have scanned for suppression attributes on the provider. This caching is necessary since we warn about duplicate suppressions. Providers without any suppressions may still be scanned multiple times since we don't cache the negative result.
| { | ||
| if (!_suppressions.TryGetValue (provider, out var suppressions)) { | ||
| suppressions = new Dictionary<int, SuppressMessageInfo> (); | ||
| suppressions = (ScannedProvider: false, Suppressions: new Dictionary<int, SuppressMessageInfo> ()); |
There was a problem hiding this comment.
Did you consider just always scanning the provider when adding a record to the cache?
It is potentially "slightly" wasteful, but it would reduce the complexity of this code.
I personally would prefer that to the "half populated" cache solution.
There was a problem hiding this comment.
I took the approach we discussed where we populate the cache for all members referenced by the assembly-level attributes. PTAL.
Now whenever we add a record to the cache, we ensure that all of the suppressions that might apply to the member have been discovered. This way we don't need to track whether we have scanned the suppressed member, but have to do a bit more work up-front.
|
@vitek-karas PTAL when you get a chance |
vitek-karas
left a comment
vitek-karas
left a comment
There was a problem hiding this comment.
Looks good.
@mateoatr please take a look as well, since you wrote the first version of this.
mateoatr
left a comment
mateoatr
left a comment
There was a problem hiding this comment.
LGTM. Thanks for adding the useful comments! Just a nit, the method access modifiers look a little bit odd on the UnconditionalSuppressMessageAttributeState class (I know this is my fault). For instance, I think the "Decode..." methods should be private. Could we change these?
For methods only used in UnconditionalSuppressMessageAttributeState
* Fix suppression check with assembly suppressions The change in dotnet/linker#2171 was incorrect because it didn't account for the possibility that the suppressions cache already contains assembly or module suppressions for the provider. This fixes the check by re-using the cache to also track whether we have scanned for suppression attributes on the provider. This caching is necessary since we warn about duplicate suppressions. Providers without any suppressions may still be scanned multiple times since we don't cache the negative result. * Populate cache eagerly to avoid extra state Now whenever we add a record to the cache, we ensure that all of the suppressions that might apply to the member have been discovered. This way we don't need to track whether we have scanned the suppressed member, but have to do a bit more work up-front. * Use private accessibility For methods only used in UnconditionalSuppressMessageAttributeState Commit migrated from dotnet/linker@5d376b1
3 participants
The change in #2171 was incorrect
because it didn't account for the possibility that the suppressions
cache already contains assembly or module suppressions for the provider.
(The same cache is used for both - the provider here is the suppression
target, not the attribute provider).
This fixes the check by re-using the cache to also track whether we have
scanned for suppression attributes on the provider. This caching is
necessary since we warn about duplicate suppressions.
Providers without any suppressions may still be scanned multiple times
since we don't cache the negative result.
I encountered this while investigating warnings in runtime, but it appears to
have been the result of an inconsistent state in my case - I don't believe
this change is necessary to fix runtime in response to the reflection warning
changes.