Block Execution of GetType() in Evaluation

[AndyGerlicher]

Stop allowing execution of GetType(). This can lead to calling methods that really should not be called during evaluation.

[AndyGerlicher]

This is needed because the type of the object in my test was System.String and in #DEBUG it thought it was an invalid ToString() call on a resource. That ended up throwing another invalid project exception which caused a stack overflow.

[jeffkl]

🤣

[AndyGerlicher]

🤣

@jeffkl Thought you might appreciate this 😄

d:\src\CBT.Core\src>d:\src\msbuild.fork\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe dirs.proj
Microsoft (R) Build Engine version 17.0.0-dev-21417-01+be92f497b for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 8/17/2021 7:31:33 PM.
Project "d:\src\CBT.Core\src\dirs.proj" on node 1 (default targets).
d:\src\CBT.Core\.build\CBT\build.props(72,26): error MSB4185: The function "GetType" on type "System.String" is not available
 for execution as an MSBuild property function. [d:\src\CBT.Core\src\dirs.proj]
Done Building Project "d:\src\CBT.Core\src\dirs.proj" (default targets) -- FAILED.


Build FAILED.

"d:\src\CBT.Core\src\dirs.proj" (default target) (1) ->
  d:\src\CBT.Core\.build\CBT\build.props(72,26): error MSB4185: The function "GetType" on type "System.String" is not availab
le for execution as an MSBuild property function. [d:\src\CBT.Core\src\dirs.proj]

    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:00.50

[rainersigwald]

This looks good and I am in favor of the change. My only concern is that it's coming a bit late in the cycle for a breaking change. IIRC we discussed this and agreed on taking it now, is that right @marcpopMSFT?

[marcpopMSFT]

I think we decided to take it as long as it made it in for preview 4 to give time for feedback given it was not particularly safe to do this. I don't think there's a good way of finding out if people are calling this already and how often.

[marcpopMSFT]

Can ya'll take a quick look through these queries to see if any critical areas are impacted (eg CBT):
https://github.com/search?q=%22gettype%28%29%22+extension%3Aprops&type=Code
https://github.com/search?q=%22gettype%28%29%22+extension%3Atargets&type=Code

Looks like a handful of impacted customers but not many.

[AndyGerlicher]

Those links look fine. I did a spot check of .csproj as well and didn't see anything, although it's harder as GetType is common to have as a file/project name.

[benvillalobos]

Should we modify the message to mention that there's a way to enable all methods with Traits.Instance.EnableAllPropertyFunctions? It would be less painful for customers this way.

[dsplaisted]

Breaking change documentation for this is here: https://docs.microsoft.com/dotnet/core/compatibility/sdk/6.0/calling-gettype-property-functions