Warnings for DAM on type can't be suppressed at the member level

[sbomer]

With the fix for #2136, we will start producing warnings for reflection access to annotated members by DAMT on types, where the warning origin will be the annotated member itself. These warnings aren't always suppressible on the member itself.

Architectural limitations make this a bit hard to solve (or maybe I'm lacking some creativity - ideas welcome!):

• We need to warn for every potential access to the member (from DAMT annotated types in this case)
• We decide whether a warning is suppressed when generating it
• We only want to scan the member's attributes once
  • And this should happen after marking the member "processed", otherwise it can trigger re-processing of the member leading to stack overflow.
  • But at the point where we process methods we have lost information about what originally caused the method to be kept

I think this points to two possible changes that would fix the suppressions:

• When we unify the DependencyInfo/ScopeStack infrastructure we should try to track enough information to decide what kind of warnings to report
• We should consider not treating warnings suppressed until we are ready to emit them, as @agocke has suggested before

[MichalStrehovsky]

I haven't been a big fan of seeing DAM processing getting moved to MarkStep and relying on marking order/DependencyInfo/ScopeStack. It doesn't translate well to the parallel processing model in NativeAOT.

A custom attribute walk is less expensive than the walk over all instructions that we're doing. It feels like we're making things more complicated just to avoid walking attributes multiple times.

E.g. just yesterday I was scratching my head how I would port #2070 to NativeAOT and I'm pretty much decided I'm just going to skip it. The extra information that we're piping through can be computed anytime we need it without having to worry we forgot to update the state (e.g. UpdateCurrentScopeInstructionOffset looks kind of like a red flag to me).

From my outside view, it looks like things took the wrong turn when we were about to do warning suppressions from compiler generated code using marking order and that approach ended up not being workable, but we left in preparatory changes that would not be needed - it feels like the design decisions in #2054 and co were largely based on the fact that we wanted marking order to do suppressions in addition to what's in custom attributes.

Now we have all these problems that whether a warning gets suppressed depends on the marking order - when the suppressions would be clear if we could just walk things and look at their attributes.

[vitek-karas]

There's another example of basically the same underlying problem: dotnet/runtime#56252
The reason why the suppression doesn't work in that case is that by default the main app assembly is "copy" and thus we blindly mark all methods in it. This time in the "Wrong" order, which is that the compiler generated code is marked before the user defined method for it (and thus we miss the suppression attribute).

I think the main problem is that warning suppression relies on the method/type/member to be marked and processed before any warnings can be generated from it. This has been known at the time this code was written, but we "wrongly" thought it's not going to be a problem. We need to change that. I think warning suppressions should not have any ties to MarkStep and if necessary they should "rescan" attributes (which is super fast and we do it for other cases like RUC already anyway).

[sbomer]

Thanks! I completely agree that marking order shouldn't change whether a warning is suppressed. If scanning attributes multiple times is OK then I think we can solve this pretty easily - we probably just need to separate the attribute marking from the scanning for suppressions. But some of the related problems @MichalStrehovsky mentioned will not be solved by this.

The extra information that we're piping through can be computed anytime we need it without having to worry we forgot to update the state

For the attribute suppressions I agree, but we do need to have some kind of context that tells us what warnings to produce. I would prefer to do this by explicitly passing in some limited local context (like a more restricted version of DependencyInfo) when calling Mark*, instead of the global ScopeStack, which tracks more than necessary and needs to be carefully kept in a consistent state. I assume that would be easier to use in parallel than a thread-local ScopeStack. Does that match your understanding?

[vitek-karas]

I'm not aware (right now) of any warning related behavior which relies on marking order - except for the suppressions. Even the compiler generated code detection should now be completely immune to random marking order. If there is such a case I think it's definitely worth filing a but for and hopefully fixing it.

I'm well aware of the current not ideal structuring of the scope stack and DependencyInfo. My plan is to change this to remove the stack and fully rely on passing parameters around. Well, just one parameter. Extend the DependencyInfo to contain functional information, not just dependency tracking information. Then we should stop using the dependency tracking information for any functional decisions and fully rely on additional fields. I'm more then open to other ideas in this space though. In short only recently I realized that we have two mechanism which do the same thing (scope stack and DependencyInfo) and that the "Stack" part is almost always covered by normal call stack. So unifying everything into DependencyInfo, passing it as parameter and thus utilize the call stack as the "Stack" is the obvious simplification of the model.

That would also address parallelism issues the current design has. I just didn't want to do it right now as it's a large refactoring (and I don't have the time and it's relatively risky).