-
Notifications
You must be signed in to change notification settings - Fork 82
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
Fix untypeast/pprintast bug (backport upstream 13845) #3663
base: main
Are you sure you want to change the base?
Conversation
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.
It seems worthwhile to test modes here? In particular, a weird thing is
let (f @ mode) = fun x -> x
is valid, but
let (f @ mode) : 'a. 'a -> 'a = fun x -> x
is not.
55fff3e
to
f227150
Compare
@tdelvecchio-jsc Good question. It turns out that the I don't have time to debug it today - what I've done is to add a test demonstrating it's broken in the first commit. The second commit doesn't fix it, but at least we know it's not the cause. I propose merging this as is and fixing this another day. |
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.
See comment, but otherwise fine with me.
pat, Some constr, modes | ||
| _ -> pat, None, [] | ||
in | ||
Vb.mk ~loc ~attrs ?value_constraint ~modes pat (sub.expr sub vb.vb_expr) |
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 currently drops vb.pvb_modes
. If you are fine with that since modes are broken anyways then that's fine, but wanted to point it out.
This tiny PR backports ocaml/ocaml#13845 to fix a bug in untypeast/pprintast.
It has already been reviewed upstream so low scrutiny is appropriate. The only question on our end is whether we need to do anything special about modes, and I think the answer is no because both
let x @ local = ...
andlet (x @ local) = ...
are valid syntax.Request review from @tdelvecchio-jsc