Skip to content

Try to extract name when backtick is missing #13369

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

Closed
wants to merge 1 commit into from

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Aug 24, 2021

Previously, completion prefix would show up as an empty string if we had an unclosed backtick. Now, we try to extract the prefix from the file contents in that case.

An alternative approach would be to tranfer somehow the name string in nme.Error, but I didn't see anything like that done before, so not sure how to properly address that.

Fixes #12514

@odersky is there a sensible way to fix this in the parser? Should we just fallback to using the name after the backtick with a reduced span?

@tgodzik tgodzik requested a review from prolativ August 24, 2021 12:56
val content = ref.source.content()
// if the error resulted from unclosed back tick
if content(ref.span.start) == '`' then
content.slice(ref.span.start + 1, ref.span.end).mkString
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wondering whether this is enough to tell that an unclosed backtick was the actual problem. Could we have a different kind of error if the backtick was closed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be enough especially if backtick is first, otherwise anything inside backticks should be ok.

@@ -842,4 +842,10 @@ class CompletionTest {
|object Main { "abc".xx${m1} }""".withSource
.completion(m1, Set())
}

@Test def wrongAnyMember: Unit = {
code"""|import scala.util.chaining.`sc${m1}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test some cases with more natural usages of backticks - if the name clashes with a keyword or contains spaces or would be normally disallowed for some other reason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some more test cases, let me know if you have any other ideas!

@tgodzik tgodzik force-pushed the fix-unclosed-backtick branch from e27d3ac to 315f7cf Compare September 24, 2021 17:42
Copy link
Contributor

@prolativ prolativ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the approval I realized that the tests don't directly cover the actual issue this PR is supposed to fix. I think you should be able to reproduce the reported scenario as a new test case in TabcompleteTests.scala

@prolativ prolativ self-requested a review September 27, 2021 09:13
@tgodzik tgodzik force-pushed the fix-unclosed-backtick branch from 315f7cf to 705d30b Compare September 27, 2021 09:52
@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 27, 2021

After the approval I realized that the tests don't directly cover the actual issue this PR is supposed to fix. I think you should be able to reproduce the reported scenario as a new test case in TabcompleteTests.scala

I added the additional tests!

@@ -138,4 +138,50 @@ class TabcompleteTests extends ReplTest {
tabComplete("import quoted.* ; def fooImpl(using Quotes): Expr[Int] = { import quotes.reflect.* ; TypeRepr.of[Int].s"))
}

@Test def wrongAnyMember: Unit = fromInitialState { implicit s =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean repeating the tests performed in CompletionTest.scala but rather directly reproducing the issue reported in #12514:
Starting with a fresh REPL try invoking completion on import scala.util.chaining.`s and then on import scala.util.chaining.`sc inside the same session

@tgodzik tgodzik force-pushed the fix-unclosed-backtick branch from 705d30b to 96b3909 Compare October 1, 2021 12:18
Previously, completion prefix would show up as an empty string if we had an unclosed backtick. Now, we try to extract the prefix from the file contents in that case.

An alternative approach would be to tranfer somehow the name string  in `nme.Error`, but I didn't see anything like that done before, so not sure how to properly address that.

Fixes #12514
@tgodzik tgodzik force-pushed the fix-unclosed-backtick branch from 96b3909 to ffc202d Compare October 1, 2021 12:19
@tgodzik
Copy link
Contributor Author

tgodzik commented Oct 1, 2021

Ok, checked it with REPL and it should work ok now.

@tgodzik
Copy link
Contributor Author

tgodzik commented Nov 8, 2021

@prolativ is taking some further look into it

@tgodzik
Copy link
Contributor Author

tgodzik commented Nov 8, 2021

Will close it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPL tab completion includes spurious Any members
2 participants