Linker warning behavior on Base/Derived and Implementation/Interface
- Dominant language
- C#
- Stars
- 392
- Forks
- 128
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 2
Description
Trimmer's initial implementation of RUC was to generate diagnostics on the called member. Due to IL-specific behavior, in general, a call to a member in a derived class is represented as a call to a member in the base class. In order to produce the same behavior in the analyzer, we decided to warn on the most base method taking into account covariant returns (see [code](https://github.com/dotnet/linker/blob/main/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs#L233-L235))
Moving forward to dataflow, there is the following scenario:
```C#
public static void Main ()
{
typeof (DerivedType).RequirePublicMethods ();
}
class BaseType
{
[RequiresUnreferencedCode ("Message for --BaseType.VirtualMethodRequires--")]
public virtual void VirtualMethodRequires ()
{
}
}
class DerivedType : BaseType
{
[RequiresUnreferencedCode ("Message for --TypeWhichOverridesMethod.VirtualMethodRequires--")]
public override void VirtualMethodRequires ()
{
}
}
```
The binder will return all the public methods in the DerivedType since is a known type, and also will return the public methods in the BaseType. After this, the trimmer will continue to `mark` the members returned by the binder and during that process, it will generate diagnostics. Currently, the diagnostic generated by the trimmer will come from the `BaseType` method, and no warning is generated for the `DerivedType` method. I think this behavior was to keep it in sync with what happens when a member is called.
Additional to this Derived/Base type behavior, there is the Implementation/Interface behavior. Given that some reflection calls can use `BindingFlags.FlattenHierarchy` the binder returns the methods from the implementation class and from the interface, for example:
```C#
public static void Main ()
{
typeof (ImplementationClass).GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy);
}
interface IRequires
{
[RequiresUnreferencedCode ("Message for --BaseType.VirtualMethodRequires--")]
public void MethodRequires ()
{
}
}
class ImplementationClass: IRequires
{
[RequiresUnreferencedCode ("Message for --TypeWhichOverridesMethod.VirtualMethodRequires--")]
public override void VirtualMethodRequires ()
{
}
}
```
The call `typeof (ImplementationClass).GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy);` returns both methods from the ImplementationClass and the IRequires interface. The diagnostic generated will come from the interface exclusively.
The analyzer will use a similar binder to the trimmer, also will understand the different intrinsic and BindingFlags (meaning that the binder should also return interface members). The difference is that the analyzer doesn't have a full concept of marking, the analyzer will attempt by default to generate diagnostics on every member returned by the binder if certain annotation conditions are met.
By default, the analyzer will try to generate warnings in both methods Derived/Base or Implementation/Interface. Where linker will only produce diagnostics in the Base or Interface.
Currently, if the analyzer wants to produce warnings in the same way as the linker additional logic needs to be implemented:
- Ignore the member if its an override
- Ignore the member if its the implementation of an interface member
Calculating if a member is an implementation of an interface member in the analyzer is an expensive process, also there are a couple of theories on if implementing this behavior is the right approach. Recently the diagnostics produced for attribute mismatch were changed to not warn on the Base/Interface and always prefer to warn on the derived/implementation because Base/Interfaces might not always be in source code.
Considering the input of various members of the team we could:
- **Generate the diagnostic taking into account the derived or implementation.** This would imply that analyzer will have to know if an interface member has an implementation member which is already defined in the roslyn API (notice that previously we had to compute from an implementation if there was a matching interface member). It still has a cost, but is less than trying to replicate what linker does. It will also prevent warnings changing places depending in the source is available for the analyzer.
- **Generate diagnostics for both derived/base or implementation/interface.** This approach will just generate warnings on every member, in C# they are considered different members so warning on both doesnt seem to be a problem. This is also the easies to implement since dataflow already has the members and can verify quickly which annotations are on them.
Considering that the warnings will always come from the `typeof(type).GetMethod()`, if someone suppressed `IL2026` is going to continue working under the two new approaches.
Related to #2418
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.