-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add Support for Name shadowing #608
Conversation
3305913
to
2526b97
Compare
2526b97
to
7f0b61b
Compare
rebased on 09/26/2023 @RexJaeschke Oddly enough, additional changes in basic-concepts.md appeared after rebasing. Let me know if we need to revert some of those changes. |
My latest commit is intended to address @BillWagner's comment. It probably also addresses @KalleOlaviNiemitalo 's comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that we need to be careful not to remove more restrictions than we want. My understanding is that shadowing is only permitted in a very few situations - you still can't write:
public void M()
{
int x = 10;
if (x == 10)
{
int x = 5;
Console.WriteLine(x);
}
}
... right? That may already be covered - I'll try to check before the meeting - but we need to make sure that it is still covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kalle's observation on nameof
is either a bug or special case handling of nameof
in declarations which is probably not clear in the Standard. If the former it has no impact here, if the latter it also has no impact here as it is the wrong place to describe it so an issue should be raised to deal with it.
Apart from that this looks good
Nigel's pretty convinced that this isn't a problem - the onus is on me to convince myself too. |
closing and reopening for a fresh CI build (test examples should pass now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triggered in error, my last review stands – approved!
Closed and reopened to check the test again... then we can merge on green. (I've created #1131 to check my earlier concern.) |
Tests are still failing - leaving assigned to @BillWagner to check up on. (Feel free to assign it back to me Bill.) |
Associated WorkItem - 250822