-
Notifications
You must be signed in to change notification settings - Fork 833
Fix 11873: Name is bound multiple times is not reported in 'as' pattern #18984
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
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes required
|
match synSimplePats with | ||
| SynSimplePats.SimplePats ([], _, m) -> | ||
// Unit (): synthesize a hidden name and unify with unit | ||
let id = ident("unitVar" + string takenNames.Count, m) |
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.
Please add a few tests showing that this won't clash with a real pattern named unitVar
, i.e. something like the following works as expected, there's no error and the patterns are not considered to be same-named:
let (unitVar, ()) = 1, ()
let f unitVar () = ()
let f (unitVar, ()) = ()
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.
Like 6375cd8 ?
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.
Could you also try to replicate the problem from the initial issue?
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.
Sure. I guess you mean adding a test for code shown in the #11873 .
let f1 a a = ()
let f2 (a, b as c) c = ()
let f3 (a, b as c) a = ()
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.
Added 3a1028b
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.
Please cover unitVar
there in the cases that are fixed here.
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.
Added in e6ea704
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.
@edgarfgp You've added many tests, but none of them cover both 1) the initial issue about different pattern groups and 2) ()
and unitVar
possibly being considered to have the same name (which would be a regression if there's a new error). This change introduces a possible regression and we need just one case to ensure it doesn't happen, but none of these additional tests seem to cover the actual change.
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.
Added in 5488c00
Description
Fixes #11873
Before
After
Checklist