Skip to content

Better positions for infix operations #1941

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 8, 2017
Merged

Better positions for infix operations #1941

merged 2 commits into from
Feb 8, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Feb 5, 2017

Review by @odersky

@smarter smarter mentioned this pull request Feb 5, 2017
Closed
@smarter
Represent untyped operators as Ident instead of Name
ac86831 
This has two advantages:
- We can distinguish BackquotedIdent from Ident, allowing the user to
  use a defined "type `&`", see testcase.
- We get better positions for the operators. This is useful in IDEs, for
  example to get the type at point.
@smarter
Better positions for infix term operations.
b0576e9 
Preserving the position of infix operators is useful for IDEs'
type-at-point. We also preserve the position of the untyped lhs of
right-associative operators, this is useful both for IDEs and for error
messages, before:
4 |val x: List[Int] = "foo" :: List(1)
  |                                   ^
  |                                   found:    String($1$)
  |                                   required: Int
  |

After:
scala> val x: List[Int] = "foo" :: List(1)
-- [E007] Type Mismatch Error: <console> ---------------------------------------
4 |val x: List[Int] = "foo" :: List(1)
  |                   ^^^^^
  |                   found:    String($1$)
  |                   required: Int
  |

Note: It would be even nicer if we displayed "String" instead of
"String($1$)" since $1$ is synthetic, this commit does not address this.
@smarter
Copy link
Member Author

smarter commented Feb 5, 2017

@felixmulder Can we get rid of validate-main? It's marked pending on all new PRs so they don't get green checkmarks

@felixmulder
Copy link
Contributor

felixmulder commented Feb 5, 2017 via email

@odersky
Copy link
Contributor

odersky commented Feb 5, 2017

I wonder whether this is not a partial solution. What do we do about definition sites, e.g.

 val `&` = ...   ?

I am asking to better evaluate whether we should fix this in the trees (which might become costly in terms of resources), or whether we should fix this by reading the source and correcting for occurrences of "`" there.

@smarter
Copy link
Member Author

smarter commented Feb 5, 2017 •
edited
Loading

This PR does several things so I'm not sure what you're pointing to with your example, could you be more specific?

@odersky
Copy link
Contributor

odersky commented Feb 5, 2017

I mean, replace Name with Ident in the ...Op nodes. That's OK as far as it goes, but if we have to do the same thing for MemberDef nodes we should discuss in more depth, and should discuss both together.

@smarter
Copy link
Member Author

smarter commented Feb 5, 2017 •
edited
Loading

OK, I see. UsingIdent in MemberDef would certainly make my job easier, but I can see how this could be costly, and going back to sources shouldn't be too hard, but I think this can be discussed separately from this PR: for infix operations, reconstructing everything from source is much harder since we don't pickle the Position.point of Select nodes (another thing which would make my job easier :)). It's also much less costly since it only affects untyped trees, and it seems necessary to not misinterpet things like Int `&` String (see testcase).

@odersky
Copy link
Contributor

odersky commented Feb 8, 2017

@smarter Yes, the explanations make sense. LGTM

@odersky odersky merged commit 99679cf into scala:master Feb 8, 2017
@allanrenucci allanrenucci deleted the fix/infix-pos branch December 14, 2017 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smarter @felixmulder @odersky