Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow GetType and compare ignoring case #9972

Conversation

f-alizada
Copy link
Contributor

Fixes #9967

Context

The GetType method is still available if called like "gettype"

Changes Made

Compare the method name ignoring the case

Testing

Added two tests

  • Cover when it is not allowed
  • Cover when GetType is enabled using env variable "MSBUILDENABLEALLPROPERTYFUNCTIONS"

Copy link
Member

@rokonec rokonec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is however functional change which can break someone.
Please consider to put it under change wave.

@f-alizada
Copy link
Contributor Author

Please consider to put it under change wave.

Thank you @rokonec for the review, the possible breaking change was discussed in the issue.
There is a documentation stating that GetType is not supported: https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/6.0/calling-gettype-property-functions
I'm not against putting the change begind the changewave however, there is a already a flag that will allow to do that:

private static bool IsInstanceMethodAvailable(string methodName)
{
if (Traits.Instance.EnableAllPropertyFunctions)
{
// anything goes
return true;
}
// This could be expanded to an allow / deny list.
return !string.Equals("GetType", methodName, StringComparison.OrdinalIgnoreCase);

What do you think?
FYI @baronfel

@rokonec
Copy link
Member

rokonec commented Apr 4, 2024

there is a already a flag that will allow to do that

@f-alizada in this case it is not a big deal. However, if I understand it correctly, we try to make it easier for customer by putting ALL functional changes into changewave and using escapehatches for long term (infinite) support of some optional features. in this particular case it make sense to me that it will be under both opt-in escape hatch and changewave.

That being said, customer could have been already broken with former case sensitive change, so maybe in this case we can leave it without changewave.

What is your take on it @rainersigwald

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an extension to an already-documented breaking change with an existing documented escape hatch I'm comfortable not putting this behind a changewave (for once!).

@f-alizada f-alizada requested a review from ladipro April 5, 2024 09:20
@f-alizada f-alizada enabled auto-merge (squash) April 5, 2024 10:06
@f-alizada f-alizada merged commit 3216460 into dotnet:main Apr 5, 2024
9 checks passed
@f-alizada f-alizada deleted the dev/f-alizada/fix-gettype-methodname-comparison branch June 14, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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