-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use other tree for actual symbol of Assign #22869
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
cbdcdd5
to
ef97824
Compare
|Also, assignment syntax can be used if there is a corresponding setter: | ||
| ${hl("def")} ${name}${hl("_=(x: Int): Unit = _v = x")} |
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.
I wouldn't add this one here. The sugaring for setters is very very scope specific and fragile. It cannot be summarized into the assignment syntax can be used if there is a corresponding setter...
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.
I think some words are necessary for user who needs an explanation; I restricted it to class members.
class ReassignmentToVal(name: Name)(using Context) | ||
extends TypeMsg(ReassignmentToValID) { | ||
def msg(using Context) = i"""Reassignment to val $name""" | ||
def msg(using Context) = i"""Reassignment to value $name""" |
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.
We could actually pass the symbol instead of the name and we would either get val
, def
,... depending on the actual symbol.
ef97824
to
76c6790
Compare
I started down the long tail of polishing words. Let the symbol say what it is, and restrict setter explanation to class-owned symbols. (Need to check class params.) |
7785a12
to
21d48c8
Compare
21d48c8
to
1187728
Compare
Updated the branch from March, which had the spree contributors. Improved to include the pathology from That test also suggested providing the args, which I took to mean show I ran out of steam and did not add the scala 2 explanation, that |
1187728
to
fb79c19
Compare
tests/neg/i22671.check
Outdated
-- [E052] Type Error: tests/neg/i22671.scala:20:4 ---------------------------------------------------------------------- | ||
20 | y = 27 // error | ||
| ^^^^^^ | ||
| Assignment to method x |
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.
Seems like the renaming isn't taken into account 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.
I think that was intentional, though it could be improved
Assignment to y which is a renaming of method x
or
Assignment to method x imported as y // closer to user syntax
fb79c19
to
a9a08b4
Compare
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.
I think this PR would profit from a discussion on concrete test cases what the error message should be. Concentrate in common test cases not obscure ones. Obscure code does not warrant much work to make clear error messages.
So, can we break out the test files into a comment, show what the current message is, and what the suggested error message should be?
tests/neg/assignments.check
Outdated
-- [E052] Type Error: tests/neg/assignments.scala:17:8 ----------------------------------------------------------------- | ||
17 | x_= = 2 // error should give missing arguments, was: Reassignment to val x_= | ||
| ^^^^^^^ | ||
| Bad assignment to setter should use x_=(2) |
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.
Sorry, I don't see how this is an improvement. Who in their right minds would write x_= =
? That's just not what I see, and we should not special case it.
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 was an existing test. And little cost to cover the case.
tests/neg/i11561.check
Outdated
3 | val updateText2 = copy(text = (_: String)) // error | ||
| ^^^^^^^^^^^^^^^^^^ | ||
| Reassignment to val text | ||
| Assignment to value text |
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.
I find "Reassignment to val" much clearer than "Assignment to value".
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.
I agree
Co-authored-by: Jan-Pieter van den Heuvel <[email protected]> Co-authored-by: Seth Tisue <[email protected]> Co-authored-by: Lucas Nouguier <[email protected]> Co-authored-by: Prince <[email protected]>
a9a08b4
to
b40dc93
Compare
The PR includes only the fix for the issue and not the semester project in psychology of programming. I think "reassignment to val" is merely familiar and corresponds to a certain mental model (that initialization or definition is one assignment, and a subsequent use of The goal was parity with Scala 2, where errors in assignment operators |
Fixes #22671
Find a name for the error message.