GetType() behave strange

[kant2002]

I attempt to annotate WinForms for linker.

I have code approximately like this

// [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] // fix
class DataGridViewCell
{
    public virtual object Clone()
    {
        DataGridViewCell dataGridViewCell = (DataGridViewCell)System.Activator.CreateInstance(GetType()); // no warning
        return dataGridViewCell;
    }
}

class DataGridViewCellConverter
{
  public override object? ConvertTo(ITypeDescriptorContext? context, CultureInfo? culture, object? value, Type destinationType)
  {
      ArgumentNullException.ThrowIfNull(destinationType);
  
      if (destinationType == typeof(InstanceDescriptor) && value is DataGridViewCell cell)
      {
          ConstructorInfo? ctor = cell.GetType().GetConstructor(Array.Empty<Type>());  // warning
          if (ctor is not null)
          {
              return new InstanceDescriptor(ctor, Array.Empty<object>(), false);
          }
      }
  
      return base.ConvertTo(context, culture, value, destinationType);
  }
}

If I add [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] to DataGridViewCell type, then issue in Clone goes away, but in Converter stays, and complain that it needs PublicParameterlessConstructor.

In my understanding these two versions of code absolutely identical and either I should have a fix for both places, or have warnings in both places.

https://github.com/dotnet/winforms/blob/5427959215dfc57d4ecf082ba5f65dc597e2c2b9/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewCell.cs#L1106-L1111
and
https://github.com/dotnet/winforms/blob/5427959215dfc57d4ecf082ba5f65dc597e2c2b9/src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewCellConverter.cs#L25-L39

[marek-safar]

/cc @vitek-karas

[vitek-karas]

This looks like a limitation of the current analysis. The analysis doesn't really track local variables as standalone values, it only tracks what is assigned into them. So in the case it sees the cell variable as having a value of "The parameter value" and the static type of that parameter is object.

A workaround for this would be to use for example local function for the code in the if branch, so something like:

      if (destinationType == typeof(InstanceDescriptor) && value is DataGridViewCell cell)
      {
          return CreateInstanceDescriptor(cell);
      }

      static object? CreateInstanceDescriptor(DataGridViewCell cell)
      {
          ConstructorInfo? ctor = cell.GetType().GetConstructor(Array.Empty<Type>());  // warning
          if (ctor is not null)
          {
              return new InstanceDescriptor(ctor, Array.Empty<object>(), false);
          }
      }

We'll look into fixing this...

[kant2002]

Will linker perserve public parameterless constructor for derived class? For example in such snippet. I do think that's not affects me, so asking for my cuiriosity.

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
public class DataGridViewCell
{
    public virtual object Clone()
    {
        DataGridViewCell dataGridViewCell = (DataGridViewCell)System.Activator.CreateInstance(GetType()); // no warning
        return dataGridViewCell;
    }
}

public class DerivedDataGridViewCell : DataGridViewCell
{
    public DerivedDataGridViewCell() {}
    public DerivedDataGridViewCell( int x ) {}
}

var x = new DerivedDataGridViewCell(5);
x.Clone();

[vitek-karas]

Yes - the annotation is applied to all derived types. This also works for interfaces, that you can put the annotation on the interface and all types implementing the interface will have that annotation applied to them.

[kant2002]

Thank you, that was not obvious to me, since all missing interface member implementations screams one me that implementation member should be annotated in same was as interface member. But for classes annotations silently applied. That's why I was a bit puzzled do I do proper thing or not.

[vitek-karas]

since all missing interface member implementations screams one me that implementation member should be annotated in same was as interface member

Could you please clarify this? Maybe an example where this happens?
The "screaming" should only happen if the implementation method doesn't have the same annotations as the interface method it implements.

[vitek-karas]

@sbomer @tlakollo @MichalStrehovsky @agocke @jtschuster - design discussion:

The problem is that MethodBodyScanner doesn't actually track local variables or stack slots as "entities", we only track external values through local variables and stack slots. So in the case of this bug, we track a value of the method parameter which is of static type object. When it gets assigned to a local variable of static type MyType we "loose" that information and we still keep tracking the method parameter with type of object.

Potential high-level solutions:

• "Typed stack" - we store static type information in every stack slot and then we use this information in the intrinsics (probably by passing another array of static types for all parameters to the HandleCall)
  • Pros - relatively simple change, can track static types of pretty much everything everywhere (since everything is a stack slot before it's actually used)
  • Cons - doesn't translate to analyzer - we would need a different solution for analyzer (I didn't look at what it would take to do it there) - but basically another "side channel".
  • Cons - relatively expensive (we track a LOT of stack slots)
• "Static typed values" - we add the ability to store static type on most SingleValue (or all on the base class?) - when there's a conversion or similar instruction we create a new value which is a clone of the input just with different static type. HandleCall would remain as is, since each value would carry its static type (similar to what we have today, it just would be more precise).
  • Pros - integrates nicely with the value system we have
  • Pros - tracks less info since in theory only some values would need to remember this, we might not need to track this information for things like UnknownValue, NullValue and so on which are very common (but see below for problems with this optimization).
  • Cons - if efficient it doesn't fully solve the problem for some edge cases - for example when we read a value from an array ldelem and we don't know what went in, we track that as UnknownValue - when used with object.GetType we would need to track the static type on the UnknownValue to fully solve this.
• "Local variable value" - we introduce a notion of local variables as tracked values. Similar to what we do for by-ref values - ldloc would return a new value LocalVariableValue which just points to the local variable. When we read from it we would get the underlying values stored there, but we would be also able to track additional static type information.
  • Pros - still integrates pretty well with value system
  • Pros - low overhead - we only add additional tracking for local values
  • Pros - potentially better warnings - in lot of places we would be able to include the name of the local variable in the warnings (if it makes sense), currently we don't have this ability
  • Cons - doesn't solve the problem for a set of cases, if for some reason the compiler decides to not create a local, or "reuse" a stack slot we would not track the static type and still fail to analyze it correctly.
  • Cons - adds complexity to intrinsics processing - we would have to make it aware of "unwrapping" (see below)
• "Typed wrappers" - the idea is very similar to "Local variable value" but for everything. Basically whenever we see a type conversion instruction we would "wrap" the input value in a StaticTypedValue (better name?) and add the static type that way.
  • Pros - very efficient since it only tracks the additional type info in places where it's needed.
  • Pros - flexible - we could add this to every place we need it (like the array element case described above for example).
  • Cons - potentially complex implementation - we would need to add understanding of this to intrinsics handling - for by-ref values we so far avoided that by unwrapping the values before they get to intrinsics processing.

Personally I would prefer a full solution over other things, so that means either "Typed stack" or "Static typed values" or "Typed wrappers". From an architecture point of view I like the "Static typed values" a bit more (it doesn't need a side channel to pass type information nor it needs "unwrapping" everywhere). The downside is that we would need to add this info on all values (so basically to SingleValue itself) and we would not have singletons for UnknownValue and NullValue anymore (since they would carry a type as well).

It's likely that "inlining" of compiler generated code will exaggerate this problem as it will apply to all of the fields on closures as well (the plan to track them as basically local variables, or something very similar).

We've discussed the idea of "wrapper" values for a while now, and actually used that for by-ref values. Maybe we should embrace it fully and swallow the associated cost - it is potentially very flexible and powerful (not only the above and by-refs, but also better warnings, easier compiler generated code and so on).

Ideas, opinions???

[vitek-karas]

dotnet/linker#2823 adds a test for this (currently expects the warning though).

[sbomer]

For me the main architectural question is whether to store the types as part of the tracked values, or as part of the containers:

• If we're only interested in tracking "static" types, then I would avoid a representation that tracks more type info than we need, so I would avoid storing types as part of the values since the local variable dictionary already has type info. We would need to add type info to the stackslots too (so the "Typed stack" approach).
• But if we want to track "dynamic" types, then I would store type info with the values, since they can have more specific type info than the statically-known variable types.

With GetType we are already using more than just the static type info: the static type of the instance is object, but the intrinsic handling relies on us passing along the "static" type of the argument in the caller. So the question is whether we want to keep that as a special case, or generalize it so that we can track "dynamic" types too.

What behavior do we want for this case?

public class C {
    public void M() {
        Base b = new Derived();
        var t = b.GetType();
        var methods = t.GetMethods(); // should this warn?
    }
}

class Base {}

[DAM(DAMT.PublicMethods)]
class Derived : Base {}

If we want to avoid warning above, I think that would eliminate the "Typed stack" approach.

The static types would probably be straightforward to track in the analyzer since every IOperation has a Type already. I don't even think we need a side-channel in the common case.

For tracking dynamic types, if we track them as part of the values this will naturally extend to the way we use values in the analyzer too. I agree that the "Local variable value" approach isn't the best since it's incomplete (and I find the concept confusing) - so for dynamic types that leaves "Static typed values" (which I would actually call "dynamic typed values" 😉 ) or "Typed wrappers".

I don't have a strong preference between the two and I would prioritize simplicity unless we have evidence that there are performance problems. FWIW I don't think the "typed wrappers" will be overly complex, and I generally like composing types from simpler types because it leverages the type system to help provide correctness. (It might make it harder to accidentally forget to take the type info into account if you have to unwrap the typed wrappers).

[agocke]

I'm not sure I follow the problem. Looking at the IL, the section that sounds like the problem is:

 .maxstack 5
        .locals init (
            [0] class DataGridViewCell cell,
            [1] class [System.Runtime]System.Reflection.ConstructorInfo ctor
        )

        IL_0000: ldarg.s destinationType
        IL_0002: ldstr "destinationType"
        IL_0007: call void [System.Runtime]System.ArgumentNullException::ThrowIfNull(object, string)
        IL_000c: ldarg.s destinationType
        IL_000e: ldtoken [System.ComponentModel.TypeConverter]System.ComponentModel.Design.Serialization.InstanceDescriptor
        IL_0013: call class [System.Runtime]System.Type [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle)
        IL_0018: call bool [System.Runtime]System.Type::op_Equality(class [System.Runtime]System.Type, class [System.Runtime]System.Type)
        IL_001d: brfalse.s IL_004a

        IL_001f: ldarg.3
        IL_0020: isinst DataGridViewCell
        IL_0025: stloc.0
        IL_0026: ldloc.0
        IL_0027: brfalse.s IL_004a

        IL_0029: ldloc.0
        IL_002a: callvirt instance class [System.Runtime]System.Type [System.Runtime]System.Object::GetType()
        IL_002f: call !!0[] [System.Runtime]System.Array::Empty<class [System.Runtime]System.Type>()
        IL_0034: callvirt instance class [System.Runtime]System.Reflection.ConstructorInfo [System.Runtime]System.Type::GetConstructor(class [System.Runtime]System.Type[])
        IL_0039: stloc.1
        IL_003a: ldloc.1

It looks like we're doing a callvirt to GetType() on ldloc.0 which has a static type of DataGridViewCell. Why is the static type object here?

[sbomer]

The problem is what @vitek-karas described here: #93720. To rephrase, we don't take into account the static type of the local even though the information is all there in IL. We just track "a value that comes from the parameter of type object", even as it's assigned to the local with a more specific type.

[vitek-karas]

Prioritization (based on offline discussion): This is not super important, except for this bug we didn't run into this problem anywhere yet. And while the workaround is not great it does exist, and the situation should be pretty rare (the usage GetType() is already pretty rare on its own).