Completion should automatically add backquotes around names that conflict with keywords

[Glavo]

scala> object O {
         val `def` = 10
       } 
// defined object O
scala> O.d 
def
scala> O.def 
1 |O.def
  |  ^^^
  |  an identifier expected, but 'def' found
scala> scala.pa 
package
scala> scala.package 
1 |scala.package
  |      ^^^^^^^
  |      an identifier expected, but 'package' found
scala> 

Sometimes we will encounter names that conflict with keywords. When we complete such names, I think we should automatically add `` prevent conflicts for these names.

[dave-kav]

Hi folks. Interested in tackling this one, will be my first time contributing. Any advice on how to approach?

[smarter]

Welcome! The code that decides which completions to show is in https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/interactive/Completion.scala, looking around quickly, it doesn't look like we have a convenience method to identifiy if a given string is a valid identifier or not, but you can easily make one from isIdentifierStart and isIdentifierPart in https://github.com/lampepfl/dotty/blob/master/library/src/scala/tasty/util/Chars.scala. Completion tests for the REPL go in https://github.com/lampepfl/dotty/blob/master/compiler/test/dotty/tools/repl/TabcompleteTests.scala, completion tests for the IDE support go in https://github.com/lampepfl/dotty/blob/master/language-server/test/dotty/tools/languageserver/CompletionTest.scala.

[dave-kav]

Perfect, thanks!

[dave-kav]

If you don't mind, can you dive a bit deeper into what the requirements are to construct a convenience method for validating identifiers from isIdentifierStart and isIdentifierPart?

[smarter]

An identifier that can be written without backquotes is one where isIdentifierStart returns true on the first character and isIdentifierPart returns true on every other character.

[dave-kav]

Perfect, just making sure there was nothing else to take into account.

[dave-kav]

To clarify - I only want to wrap the name in backticks if it is a valid identifier but clashes with a keyword, correct?

[smarter]

a name needs to be wrapped in backticks if it's not a valid identifier, which is the case when it's a keyword or when it can only be written with backticks (e.g. if you write object Foo { val a b = 1 } and do completion on Foo. you want to get back Foo.`a b` )

[dave-kav]

Is there a convenience method to determine whether an arbitrary string is a keyword in existence yet? I see all keywords defined in package dotty.tools.parsing.Tokens, maybe the allTokens: TokenSet will be useful if not.

[smarter]

I don't think there is one.

[dave-kav]

Ok, thanks for responding so quickly! I've made some changes and added the following test to TabCompleteTests:

@Test def tabCompleteWithBackTicks = {
    val src1 = "object Foo { val `def` = 1 }"
    val src2 = "Foo.d"

    fromInitialState { implicit state =>
      run(src1)
    }
    .andThen { implicit state =>
      val expected = List("`def`")
      assertEquals(expected, tabComplete(src2))
    }
}

This passes, but I've taken a couple of tests out with the change:
[error] Test dotty.tools.repl.TabcompleteTests.anyRef failed: java.lang.AssertionError: expected:<List(!=, ##, +, ->, ==, asInstanceOf, clone, ensuring, eq, equals, finalize, formatted, getClass, hashCode, isInstanceOf, ne, notify, notifyAll, synchronized, toString, wait, →)> but was:<List(`!=`, `##`, `+`, `->`, `==`, `→`, asInstanceOf, clone, ensuring, eq, equals, finalize, formatted, getClass, hashCode, isInstanceOf, ne, notify, notifyAll, synchronized, toString, wait)>, took 0.164 sec [error] at dotty.tools.repl.TabcompleteTests.$anonfun$anyRef$1(TabcompleteTests.scala:133) [error] at dotty.tools.repl.TabcompleteTests.$anonfun$anyRef$1$adapted(TabcompleteTests.scala:128) [error] at dotty.tools.repl.ReplTest.fromInitialState(ReplTest.scala:35) [error] at dotty.tools.repl.TabcompleteTests.anyRef(TabcompleteTests.scala:128) [error] ...
I need to cater for these operators. Are there definitions I can reference or is this going to require a more manual approach?

[smarter]

Ah, looks like isIdentifierStart/Part are not enough indeed, looks like you need to use isOperatorPart too. For the exact rules to form a valid identifier see section 1.1 of the spec: https://www.scala-lang.org/files/archive/spec/2.12/01-lexical-syntax.html

[dave-kav]

Thanks!

[dave-kav]

Actually, I don't think the problem was with not using isOperatorPart but rather in how I've affected the behaviour. In the case where a name conflicts with an operator type or keyword do we wish to present both options, or just the escaped version?

[smarter]

I'm not sure I understand your question, in Scala users are free to define operators like def -> = ... , and call them using -> without backquotes.

[dave-kav]

Sure, I understand that. I'm more wondering if there's a case where there can be 2 valid symbols in scope, where we capture one in backticks but both are valid completions?

[smarter]

Can you give a full example of the case you have in mind ?