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

Pattern discard not allowed for union case that takes no data #14055

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Oct 8, 2022

Fixes #13851

  • Put this behind a Lang preview

@edgarfgp edgarfgp closed this Oct 8, 2022
@edgarfgp edgarfgp reopened this Oct 8, 2022
@abelbraaksma
Copy link
Contributor

Great work! This also fixes the (closed with ‘resolution by design’) issues: #10204 and #1592.

@edgarfgp edgarfgp marked this pull request as ready for review October 9, 2022 10:48
@T-Gro
Copy link
Member

T-Gro commented Oct 10, 2022

/azp run

T-Gro
T-Gro previously approved these changes Oct 10, 2022
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro T-Gro requested review from psfinaki, 0101 and abonie October 10, 2022 08:43
@abonie
Copy link
Member

abonie commented Oct 10, 2022

Does this also cover function keyword? If so, it would make sense to add a unit test for that too

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Oct 10, 2022

Does this also cover function keyword? If so, it would make sense to add a unit test for that too

Yes, it does, I have added more tests. Thanks for the review :)

@T-Gro T-Gro self-requested a review October 10, 2022 12:12
T-Gro
T-Gro previously approved these changes Oct 10, 2022
@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 10, 2022

Does this also cover function keyword? If so, it would make sense to add a unit test for that too

Yes, it does, I have added more tests. Thanks for the review :)

Does it affect single-case unions when using them as a deconstruct syntax in functions? I.e. was it allowed before and is it now? My guess it shouldn't be affecting it, since they'll just be aliases?

@T-Gro
Copy link
Member

T-Gro commented Oct 10, 2022

It was working before (with _ discard), and was not working before when using named arg.
@edgarfgp : Can you please test what the new behavior is?

IMO it would be more consistent with other features it function arguments also show a warning if empty discard is used, but the opinion is not very strongly held.
image

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Oct 10, 2022

Does it affect single-case unions when using them as a deconstruct syntax in functions? I.e. was it allowed before and is it now? My guess it shouldn't be affecting it, since they'll just be aliases?

Can you give an example in code to better illustrate your question :) ?

@T-Gro
Copy link
Member

T-Gro commented Oct 10, 2022

It was working before (with _ discard), and was not working before when using named arg. @edgarfgp : Can you please test what the new behavior is?

IMO it would be more consistent with other features it function arguments also show a warning if empty discard is used, but the opinion is not very strongly held. image

FYI the middle ones gives a FS0725 error (good), and it gives a fixing suggestion of replacing this with an underscore.
=> with this PR, would be good to NOT suggest putting in an underscore in case of empty field list.
(or the suggestion would rather remove the field alltogether, instead of replacing it with an underscore)

image

@edgarfgp
Copy link
Contributor Author

It was working before (with _ discard), and was not working before when using named arg. @edgarfgp : Can you please test what the new behavior is?

@T-Gro Ok let me add a test with your code sample :)

IMO it would be more consistent with other features it function arguments also show a warning if empty discard is used, but the opinion is not very strongly held. image

Agreed would be good to show the new warning error to make it consistent

FYI the middle ones gives a FS0725 error (good), and it gives a fixing suggestion of replacing this with an underscore. => with this PR, would be good to NOT suggest putting in an underscore in case of empty field list. (or the suggestion would rather remove the field alltogether, instead of replacing it with an underscore)

image

I guess this quick fix is not longer needed and a new/ or existing one suggesting remove the _ will be more useful
I would not be helpful with any VS tooling(quick fixes) as part of this PR

Any help is appreciated ;)

@vzarytovskii
Copy link
Member

I guess this quick fix is not longer needed and a new/ or existing one suggesting remove the _ will be more useful\nI would not be helpful with any VS tooling(quick fixes) as part of this PR

If you could just create a follow-up issue for removing exciting quick fix for this case and adding new one (remove discard) - we'll take care of it.

psfinaki
psfinaki previously approved these changes Oct 10, 2022
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Hah I was scared to review this due to the number of files in the PR but then realized it's actually the analyzer fixing code in our repo :D

Thanks, great work - and especially great testing :)

src/Compiler/FSComp.txt Outdated Show resolved Hide resolved
@edgarfgp edgarfgp dismissed stale reviews from psfinaki and T-Gro via 286098a October 10, 2022 16:48
@edgarfgp
Copy link
Contributor Author

Hah I was scared to review this due to the number of files in the PR but then realized it's actually the analyzer fixing code in our repo :D

Thanks, great work - and especially great testing :)

Thanks, Happy to help

Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

Sweet, thanks a ton for it, Edgar, great job as usual!

@vzarytovskii vzarytovskii enabled auto-merge (squash) October 10, 2022 17:38
@vzarytovskii vzarytovskii merged commit 26bc85e into dotnet:main Oct 10, 2022
@edgarfgp
Copy link
Contributor Author

Sweet, thanks a ton for it, Edgar, great job as usual!

Thanks for the kind words :) . It always a pleasure to help F# to get better.

I’m already looking for the next issue to contribute

@psfinaki
Copy link
Member

@edgarfgp here are some ideas for you ;)

@abelbraaksma
Copy link
Contributor

Awesome that you tackled this long standing issue! 🥳

@edgarfgp
Copy link
Contributor Author

My pleasure @abelbraaksma , @NinoFloris might also like this one as he raised this issue as well 😀

@edgarfgp
Copy link
Contributor Author

@edgarfgp here are some ideas for you ;)

Thanks @psfinaki . I have most of those issues on my radar . I might ask for some technical guidance in some of them

@psfinaki
Copy link
Member

@edgarfgp sure - absolutely :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Pattern discard allowed for union case that takes no data
6 participants