-
-
Notifications
You must be signed in to change notification settings - Fork 115
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(compiler): Fix array infix assign semantics #2080
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.
This looks great, as a note my pr that implemented infix formatting still serves a role as in cases like:
arr[1] = arr[1] + 1
We are still going to want the formatter to convert those to infix automatically.
I didn't implement that because of the |
706f721
to
d27d4d4
Compare
Detecting the simple case of an identifier or constant is easy I just don't think we should go beyond that . |
I'll do that in a follow-on PR. |
Sounds good |
I'll also do a follow-on PR to use this same format for regular infix assignment and record infix assignment. |
Error( | ||
infix.exp_loc, | ||
env, | ||
Arity_mismatch(expand_head(env, infix.exp_type), None), |
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.
Shouldn't this be an Impossible
?
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's not, because you can redefine what the infix operators do, and we don't stop you from making one take a number of arguments other than two. (You wouldn't be able to call it with the infix syntax, but you could call it as a regular function.)
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.
LGTM
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.
Already merged, but also looks good to me
Closes #1988