[Bug]: GetType can still be called as property function, due to case-sensitive comparison

[KalleOlaviNiemitalo]

Issue Description

#6769 supposedly prevented calling GetType() as a property function, but it does not recognize the name if written with different letter case, e.g. gettype.

Steps to Reproduce

demo.proj:

<Project>
  <PropertyGroup>
    <foo>aa</foo>
  </PropertyGroup>

  <Target Name="bb">
    <Message Importance="high" Text="$(foo.gettype().FullName)"/>
  </Target>
</Project>

dotnet msbuild demo.proj

Expected Behavior

$ dotnet msbuild demo.proj
MSBuild version 17.9.6+a4ecab324 for .NET
demo.proj(7,32): error MSB4185: The function "GetType" on type "System.String" is not available for execution as an MSBuild property function.

Actual Behavior

$ dotnet msbuild demo.proj
MSBuild version 17.9.6+a4ecab324 for .NET
  System.String

Analysis

Perhaps a case-insensitive comparison here would fix it:

msbuild/src/Build/Evaluation/Expander.cs

Line 5287 in a4ecab3

return methodName != "GetType";

Alternatively, compare the MemberInfo.Name string after the lookup, rather than the user-specified string. That might be a larger change, though.

Versions & Configurations

MSBuild version 17.9.6+a4ecab324 in .NET SDK 8.0.202

[rainersigwald]

Nice find! I can't believe that match is not case sensitive in the first place.

[KalleOlaviNiemitalo]

Will the fix be considered a breaking change again?

[rainersigwald]

I'm inclined to say no--because "we already updated the documentation to say you can't do that" but it's definitely a concern. @baronfel do you have thoughts?

[KalleOlaviNiemitalo]

Another possible fix would be to block access to System.Type references in general, rather than just GetType().

[baronfel]

I'm inclined to say 'no' for the same reason - the justification for the breaking change notes that there is no broadly-known usage of GetType any longer, so I think the impact of this would be quite low. If we get any significant user feedback that it is in use we can of course add breaking change notices quite quickly and out-of-band of the MSBuild code itself.

[f-alizada]

If there is a possibility of low usage, shall we put the change behind the changewave?

[f-alizada]

I see :) We have the MSBUILDENABLEALLPROPERTYFUNCTIONS, so the need of changewave is lowered.
Had spent some time understanding why it is not reproducible in debug mode :

msbuild/src/Framework/LazyFormattedBuildEventArgs.cs

Line 183 in 28f488a

if (param != null && param.ToString() == param.GetType().FullName)

[KalleOlaviNiemitalo]

Does that InvalidOperationException also trigger for param == "System.String"? If so, that seems misguided.

"System.String".ToString() == "System.String".GetType().FullName

[f-alizada]

@KalleOlaviNiemitalo it was exactly the case.

[KalleOlaviNiemitalo]

Oh, I assumed you had param == typeof(RuntimeType), thus typeof(RuntimeType).ToString() == typeof(RuntimeType).GetType().FullName.

[KalleOlaviNiemitalo]

I imagine && !(param is string) would get rid of the false "Invalid type for message formatting argument" error.

[f-alizada]

To be honest, I was thinking to leave it as it is, since it is in debug mode + the fix is prepared.
I'm not 100% sure that this fix should be applicble only for string, however did not investigate details, there could be cases when ToString method overriden and returns the value == param.GetType().FullName, since the arguments is an array of object object[] args.