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

Remove extra logic to see if users are squatting on keywords #59086

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

I personally would consider this a regression of an existing good behavior. I think this makes a semantic classifier not semantic-aware.

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

I have no objection to updating the syntax classifier to preemptively classify var as a keyword, but the semantic classifier must correct the classification for cases where it's not used in that form.

@CyrusNajmabadi
Copy link
Member Author

I personally would consider this a regression of an existing good behavior. I think this makes a semantic classifier not semantic-aware.

I'm fine with taht. Indeed, i'll just move this to syntactic classification.

@CyrusNajmabadi
Copy link
Member Author

I have no objection to updating the syntax classifier to preemptively classify var as a keyword,

I'll do that.

but the semantic classifier must correct the classification for cases where it's not used in that form.

There is no need for the semantic classifier to do anything with htis. We're effectively stating at the language levle that we don't care about this sort of user code. It's non-grata. The IDE operates under an even looser set of rules, and we have absolutely no need or obligation to spend extra cycles trying to provide specialized behavior against pathological code.

@CyrusNajmabadi CyrusNajmabadi dismissed sharwell’s stale review July 18, 2023 22:25

LDM position is that users creating types with the same name as lower-cased lang keywords is not something we care about supporting (esp. not for tooling that does not have any compat requirements).

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

This is an unnecessary and harmful change. The original behavior is more correct with no articulated problems in performance or maintainability.

@CyrusNajmabadi
Copy link
Member Author

with no articulated problems in performance or maintainability.

The articulated problem is complexity (and that is the case in lots of the code, not just this). We have lots and lots of code that attempts to effectively work with code that we as a team are stating we now consider to be pathological.

The sentiment is that this is not something we are going to continue with, and we will even actively be pushing people away from (including breaking if it comes to that).

This change simplifies things, allowing us to do less work, and to not have to waste dev cycles (and user cycles) for pathological cases.

@sharwell
Copy link
Member

I would of course be fine with this change being made if/when the compiler starts rejecting this code as a compilation error. One of the things that sets Visual Studio / C# IDE apart from many other IDEs is we actually do care about correctness. Just because we don't like the way some piece of code looks doesn't mean we can simply ignore it if it falls under what the language does.

@CyrusNajmabadi
Copy link
Member Author

is we actually do care about correctness.

Our position is often that we are not going to be correct, or spend extra effort, on the pathological. Most (all?) of our features do not successfully work in those cases. But the effort and added complexity into our code to support things like that is just not worth it.

We care about high correctness in the real world. And we explicitly do not care, or want to cater to, people doing pathological stuff. A team that has a type named var is not a good .net citizen, and we actively should not be expending effort, or adding code complexity, to support them.

@CyrusNajmabadi CyrusNajmabadi merged commit 14a70ca into dotnet:main Nov 22, 2023
@ghost ghost added this to the Next milestone Nov 22, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the specialCaseKeywords branch November 22, 2023 02:07
@RikkiGibson RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants