-
Notifications
You must be signed in to change notification settings - Fork 473
Better syntax errors for [| and |>
#8008
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
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
tests/syntax_tests/data/parsing/errors/expressions/expected/oldDataLastPipe.res.txt
Outdated
Show resolved
Hide resolved
mediremi
left a 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.
Nice work!
| base | ||
| ^ "\n\n\ | ||
| \ The old data-last pipe `|>` has been removed from the language.\n\ | ||
| \ Use a data first `->` pipe instead." |
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.
Not sure if an LLM would understand this.
Did you test this error message?
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.
Yes, it works well in my tests.
| I'm not sure what to parse here when looking at "|". | ||
|
|
||
| The old data-last pipe `|>` has been removed from the language. | ||
| Use a data first `->` pipe instead. |
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.
Those are not interchangeable, so as a user if I natively just swap |> with ->, I'm just heading for the next error.
String.length example only works because there is one argument right?
Might be good to have an example with two or more so it becomes clear how to deal with this.
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.
Correct. I made a small wording change to call it "Refactor to use a ..." instead of "Use a ..." to signify that some form of action is needed. Given it's removed from the language, not just deprecated, I think it's enough. LLMs understand it anyway because they know what -> vs |> is. And, users who end up in this situation can continue to the next error and have something to Google for if they need more info.
| I'm not sure what to parse here when looking at "|". | ||
|
|
||
| The old data-last pipe `|>` has been removed from the language. | ||
| Refactor to use a data-first `->` pipe instead. |
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.
let x = "x" |> String.replace("abc", "a")
let y = "x" -> String.replace("abc","a", _)May have also been relevant to add. Not sure if that always works though.
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.
Yeah, but it's quite tricky what to suggest imo. That certainly works, but it implies continuing to use a data last API, which we don't really want to promote. But if this turns out to be an issue we can mull a bit on what example to use, if any.
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.
That is fair though, we do want to move away from this, so that makes sense.
I keep hitting especially
[|when doing agentic coding. A simple suggestion in the error message like the one in this PR should help the agent self-heal.I wanted to make this as non-invasive as possible, hence where this is implemented. So I didn't want to make the parser actually understand these patterns again, just do a best-effort suggestion if it's likely that what is failing is trying to use these old patterns.