Topic/533 parse error #1847

b-studios · 2016-12-21T16:26:22Z

This pull request deals with "Parsers.scala:533". It is part of #1589.

Message Before

-- Error: examples/parser533.scala ---------------------------------------------
3 |} // error
  |^
  |`.' expected
-- [E025] Syntax Error: examples/parser533.scala -------------------------------
2 |  type X = FooBar22.this // error
  |           ^^^^^^^^^^^^^
  |           identifier expected

Explanation
===========
An identifier expected, but `FooBar22.this` found. This could be because
`FooBar22.this` is not a valid identifier. As a workaround, the compiler could
infer the type for you. For example, instead of:

def foo: FooBar22.this = {...}

Write your code like:

def foo = {...}



two errors found

Message After

-- [E036] Syntax Error: examples/parser533.scala -------------------------------
2 |  type X = FooBar22.this // error
  |           ^^^^^^^^^^^^^
  |           Expected an additional member selection after the keyword this

Explanation
===========
Paths of imports and type selections must not end with the keyword this.

Maybe you forgot to select a member of this? As an example, in the
following context:
  trait Outer {
    val member: Int
    type Member
    trait Inner {
      ...
    }
  }

- this is a valid import expression using a path
  import Outer.this.member
  //               ^^^^^^^

- this is a valid type using a path
  type T = Outer.this.Member
  //                 ^^^^^^^


one error found

In addition to adding the required error message this pull request also fixes some inconsistencies with referring to identifier literals. While in the doc-comments one could find Ident, ident, ident(), Id and id, in the spec identifiers are always referred to as id.

b-studios · 2016-12-21T21:24:37Z

@felixmulder @Blaisorblade I am happy to fix the conflicts, once the PR has been reviewed.

felixmulder · 2016-12-21T21:38:45Z

Paolo is probably a good reviewer for this PR in my opinion :) so I'll delegate to him ;)

Blaisorblade · 2016-12-22T17:32:19Z

I'll try but I can't reply very timely these days...

b-studios · 2016-12-23T13:20:27Z

Sorry @Blaisorblade, for a split-second while mentioning you, I forgot how busy you are these days. Maybe someone else can review this PR? Cheers and happy holidays!

Blaisorblade · 2017-01-07T12:05:58Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -901,4 +901,31 @@ object messages {
    val msg = hl"trying to define package with same name as `$existing`"
    val explanation = ""
  }
+
+  case class DangelingThisInPath()(implicit ctx: Context) extends Message(34) {


Dangeling -> Dangling. (And given the delay, maybe recheck the ID 34).

Indeed we're up to 35, so right now you'll need a 36. https://github.com/lampepfl/dotty/blob/bf9888edcd96f40f11260e6ad874eb27382f77ba/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala#L926

Blaisorblade · 2017-01-07T12:08:54Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+    val msg = hl"""Expected an additional member selection after the keyword ${"this"}"""
+
+    val importCode =
+      """import MyClass.this.member


Maybe use OuterClass instead of MyClass? And even add class OuterClass { class InnerClass { ... } } as context to show the point of MyClass.this? I ask because it took me a (small) while to get the point MyClass.this (I'll concede I might also be rusty).

Blaisorblade · 2017-01-07T12:13:40Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -504,7 +504,7 @@ object Parsers {
    def selector(t: Tree): Tree =
      atPos(startOffset(t), in.offset) { Select(t, ident()) }

-    /** Selectors ::= ident { `.' ident()
+    /** Selectors ::= id { `.' id }


Good commit. Possible further idea: mention id in the comment for isIdent, also given they're not consistent? So authors of other comments can also find the right terminal name to use in comments?
(Well, being consistent might be even better but I'm not sure what to pick).
But that shouldn't hold this PR.

Blaisorblade · 2017-01-07T12:16:46Z

LGTM apart from the comments (and even those are mostly optional) — sorry for the delay, I hadn't realized the review would be so quick!

b-studios · 2017-01-07T15:29:38Z

@Blaisorblade Thanks for the review. I provided some more context to the code examples in 21194ba

Blaisorblade · 2017-01-08T12:59:54Z

The CI failure is on http://dotty-ci.epfl.ch/lampepfl/dotty/475/1 at line 6164-6173.* That seems to have to do with a testcase from 18bc638 in #1850 — for some reason only 1 error is being output instead of 2.

Does this relate to switching error message from strings, or (in some complicated way) to adding , start as argument to syntaxError? I'm hoping the latter, but no clue why except if the if is skipped in
https://github.com/b-studios/dotty/blob/065377cadd9634b6fb48da561f1f504082fd06f5/compiler/src/dotty/tools/dotc/parsing/Parsers.scala#L111-L112.

BTW, with that change the caret is not on this (line 6167 in logs), and I'm not sure I like that.

*OT for @jvican, assuming you picked Drone: I can't find a way to link to specific log lines in Drone logs like I'd do in these cases with Travis. Am I missing something? Or any chance you could please forward this feature request to the right people?

b-studios · 2017-01-08T13:49:26Z

I verified that, in deed, only one error is produced in presence of syntaxError(DanglingThisInPath(), start), while there are two with syntaxError(DanglingThisInPath()). Here are the two errors, of which one is missing:

-- [E036] Syntax Error: examples/parser533.scala -------------------------------
3 |}
  |^
  |Expected an additional member selection after the keyword this

longer explanation available when compiling with `-explain`
-- [E025] Syntax Error: examples/parser533.scala -------------------------------
2 |  type X = FooBar22.this // error
  |           ^^^^^^^^^^^^^
  |           identifier expected
longer explanation available when compiling with `-explain`

felixmulder · 2017-01-08T13:50:56Z

@Blaisorblade, @b-studios, haven't looked too closely at this, but FYI:

Error messages are pruned by UniqueMessagePositions.scala and HideNonSensicalMessages.scala. If positions are changed when modifying error messages, this is where they would be pruned out.

The caret is placed on the point of a position if it is focused, otherwise it will be a sort of underline of carets from pos.start to pos.end.

Regarding Drone, http://discourse.drone.io/ is probably the appropriate place to make a feature request. They also have a gitter: https://gitter.im/drone/drone

Cheers,
Felix

b-studios · 2017-01-08T13:55:46Z

Thanks @felixmulder for the caret explanation. Changing the error call to syntaxError(DanglingThisInPath(), t.pos) will print as

2 |  type X = FooBar22.this // error
  |           ^^^^^^^^^^^^^
  |           Expected an additional member selection after the keyword this

@Blaisorblade Do you like that better?

However, the second error message "identifier expected" is still pruned away since it is located at the same message position.

Blaisorblade · 2017-01-08T14:55:07Z

However, the second error message "identifier expected" is still pruned away since it is located at the same message position.

FWIW, looking at the two messages, I'd say pruning away "identifier expected" is an improvement :-), and the new position is much better.
In the future, I'd show the old error in the commit message to clarify the goal of the change.

TL;DR. I'd remove the second // error in the testcase (I'm guessing that comment marks expected errors), get a green build and merge.

Regarding Drone, http://discourse.drone.io/ is probably the appropriate place to make a feature request. They also have a gitter: https://gitter.im/drone/drone

Thanks, done here:
http://discourse.drone.io/t/feature-request-allow-links-to-specific-log-lines/150

b-studios · 2017-01-08T17:34:42Z

@felixmulder Build now is successful. Is there something more I can do (like squashing, etc.), or are we ready to merge?

felixmulder · 2017-01-08T18:14:03Z

I'd like the merge commit rebased away - otherwise LGTM 👍

The following examples trigger the error message: val x: Foo.this = ??? // Also triggers the error: import foo.this // Additionally, also slays the compiler type X = Foo.this.type

To match the specs in https://github.com/lampepfl/dotty/blob/master/docs/syntax-summary.txt all occurences of Id, ident or Ident in comments have been replaced with the terminal `id`.

b-studios · 2017-01-08T19:09:29Z

@felixmulder I rebase the merge commit away and also squashed the typo-commit.

felixmulder · 2017-01-08T19:11:07Z

Great, thanks a bunch! You're awesome 🎉

b-studios mentioned this pull request Dec 22, 2016

Topic/533 parser error b-studios/dotty#1

Closed

Blaisorblade reviewed Jan 7, 2017

View reviewed changes

dottybot added the reviewed label Jan 7, 2017

b-studios force-pushed the topic/533-parse-error branch 2 times, most recently from d74f40f to bc8af8f Compare January 8, 2017 18:34

b-studios added 4 commits January 8, 2017 19:37

Add error message for dangling this in path selections

fa9fed6

The following examples trigger the error message: val x: Foo.this = ??? // Also triggers the error: import foo.this // Additionally, also slays the compiler type X = Foo.this.type

Replace all occurrences of Id, ident or Ident with id

ab91bd5

To match the specs in https://github.com/lampepfl/dotty/blob/master/docs/syntax-summary.txt all occurences of Id, ident or Ident in comments have been replaced with the terminal `id`.

Give context for code examples to ease understanding

09ace25

Improve error position and drop second error since it is pruned

feba6a4

b-studios force-pushed the topic/533-parse-error branch from bc8af8f to feba6a4 Compare January 8, 2017 18:37

felixmulder merged commit 7113db7 into scala:master Jan 8, 2017

felixmulder removed the reviewed label Jan 8, 2017

Topic/533 parse error #1847

Topic/533 parse error #1847

Uh oh!

Conversation

b-studios commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Message Before

Message After

Uh oh!

b-studios commented Dec 21, 2016

Uh oh!

felixmulder commented Dec 21, 2016

Uh oh!

Blaisorblade commented Dec 22, 2016

Uh oh!

b-studios commented Dec 23, 2016

Uh oh!

Blaisorblade Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

Blaisorblade Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

Blaisorblade Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

Blaisorblade Jan 7, 2017

Choose a reason for hiding this comment

Uh oh!

Blaisorblade commented Jan 7, 2017

Uh oh!

b-studios commented Jan 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blaisorblade commented Jan 8, 2017

Uh oh!

b-studios commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixmulder commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b-studios commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blaisorblade commented Jan 8, 2017

Uh oh!

b-studios commented Jan 8, 2017

Uh oh!

felixmulder commented Jan 8, 2017

Uh oh!

b-studios commented Jan 8, 2017

Uh oh!

felixmulder commented Jan 8, 2017

Uh oh!

Uh oh!

b-studios commented Dec 21, 2016 •

edited

Loading

b-studios commented Jan 7, 2017 •

edited

Loading

b-studios commented Jan 8, 2017 •

edited

Loading

felixmulder commented Jan 8, 2017 •

edited

Loading

b-studios commented Jan 8, 2017 •

edited

Loading