Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce a DiscardSyntaxClassifier #40396
Introduce a DiscardSyntaxClassifier #40396
Changes from 22 commits
543803d
31feeb8
e6d2199
2bed547
8bd63d9
d23ec64
8786800
3552b39
e71d97c
03b7756
d366c3c
8e93d34
bf47f1b
74d8060
3a493ea
8b77049
16ba315
d498eb8
25900e9
3bed577
7bdbfb9
5ab4a04
b948f05
e914514
2122217
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
tests with tuples?
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.
Should the "_" be highlighted for tuples? Currently they aren't and the original Issue only talked about arguments.
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.
IMO, yes. If we're going to classify discards this way, we should consistently do it everywhere they come up.
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.
Ok. In that case i could use the new implementation and make it deal with
DiscardPatternSyntax
andDiscardDesignationSyntax
which, according to the Synatx.xml should be the only two types we actually need to deal with.Do we have some kind of syntax generator that i can use to generate test cases for most types of syntax constructs containing discards?
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.
WFM.
Yes. He's called @jcouv :)
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.
Here are some more scenarios to test (different contexts where discards are allowed):
bool found = TryGetValue(out var _)
orbool found = TryGetValue(out _)
(x, _) = deconstructable;
(var x, var _) = deconstructable;
x is int _
case int _:
(_, _) => body;
or(int _, int _) => body;
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.
@jcouv Was tehre any work done to make
_
in a lambda a discard?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.
There was a little: #38786
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.
First of all, thank you for providing a list of test cases.
Is this actually a discarding case? for me this
Func<int, int, int> a = (int _, int __) => { return _ + __; };
compiles and I can use the "discard" in the body. I also get a CS0100 if i define it whith two single underscores.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.
This is not a discarding case:
Func<int, int, int> a = (int _, int __) => { return _ + __; };
But this is:
Func<int, int, int> a = (int _, int _) => { return /* cannot reference either discards here */; };
. This requires language version Preview (otherwise it's a compilation error).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.
FWIW, to recognize discard parameters in lambdas, the key API is
IParameterSymbol.IsDiscard
.