Skip to content
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

bugfix: rename end marker for local definitions #5803

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented Oct 31, 2023

resolves: #5797

@kasiaMarek kasiaMarek changed the title bugfix: rename end marker for lolac definitions bugfix: rename end marker for local definitions Nov 2, 2023
@kasiaMarek kasiaMarek marked this pull request as ready for review November 2, 2023 09:01
Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

LGTM

isDeclaration = isDeclaration(tree),
)
)
case _ => None
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can keep the symbol in end marker and this way provide good semantic token for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. By "good" we mean that it should be the same as the definition it ends?

Copy link
Member

Choose a reason for hiding this comment

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

exactly

@@ -493,6 +493,25 @@ abstract class PcCollector[T](
case df: NamedDefTree
if df.span.isCorrect && df.nameSpan.isCorrect &&
filter(df) && !isGeneratedGiven(df) =>
def collectEndMarker =
val name = df.name.toString()
val endMarker = s"end $name"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this potentially break if we have more spaces between end and name ?

Copy link
Contributor

@filipwiech filipwiech Nov 2, 2023

Choose a reason for hiding this comment

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

Or a trailing whitespace/comment at the end of the line? 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use regex instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid createc new regexes hre, we can just split of \s+ and check the two resulting parts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory there can also be a comment there. Regex would take care of that too. Are you worried about the performance? I think it should be fine, since it would be a simple regex. But I can parse it by hand too.

@@ -488,4 +488,12 @@ class PcRenameSuite extends BasePcRenameSuite {
| } yield b
|""".stripMargin,
)

check(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be a good idea to add a test where the end marker has incorrect (not matching) name and shouldn't be renamed? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! 👍

@kasiaMarek kasiaMarek force-pushed the pc-end-marker-rename branch from a09c27a to 43c996b Compare November 2, 2023 12:52
object EndMarker/*example.EndMarker.*/:
def foo/*example.EndMarker.foo().*/ =
1
end foo
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 feels like we should detect example.EndMarker.foo() here, since semanticdb does that.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are mostly needed for definition so that should be fine.

)

check(
"end-marker-wrong",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"end-marker-wrong",
"end-marker-wrong".tag(IgnoreScala2),

Missing test tag? 🙂

"end-marker-with-comment".tag(IgnoreScala2),
"""|def <<he@@llo>>(a: Int) =
| ???
|end /* a comment */ <<hello>> /* a comment */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
|end /* a comment */ <<hello>> /* a comment */
|end /* a comment */ <<hello>> // a comment

Nit, to test both types of comments. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't matter here, that comment is outside of definition span anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. 👍 As somebody not that knowledgeable about the inner workings I still found it useful (test showing the expected behaviour without any assumptions about the internal implementation). 😉

@@ -70,7 +70,7 @@ abstract class PcCollector[T](
case _ => rawPath
def collect(
parent: Option[Tree]
)(tree: Tree, pos: SourcePosition, symbol: Option[Symbol]): T
)(tree: Tree | EndMarker, pos: SourcePosition, symbol: Option[Symbol]): T
Copy link
Contributor

@filipwiech filipwiech Nov 2, 2023

Choose a reason for hiding this comment

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

Sorry if the question doesn't make much sense, I'm not familiar with how these things work, but could end marker be a part of the tree? Is there a reason to have it be a separate entity? 🤔

Copy link
Contributor Author

@kasiaMarek kasiaMarek Nov 2, 2023

Choose a reason for hiding this comment

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

I have it separately because it's not a subtype of Tree (from dotty compiler) but a special case class I created. End markers don't exist in abstract syntax trees, they are pruned since in a sense they provide no value there. I'm not sure if that answers your question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thank you for the explanation! 👍

@kasiaMarek kasiaMarek force-pushed the pc-end-marker-rename branch from 43c996b to d9a926d Compare November 2, 2023 13:36
): Option[SourcePosition] =
val name = df.name.toString()
val endMarkerLine =
sourceText.slice(df.span.start, df.span.end).split('\n').last
Copy link
Contributor

Choose a reason for hiding this comment

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

This has popped up recently a few times already: is just \n enough, or would it make sense to check other newline combinations as well? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't matter (unless someone has random carriage returns in code).

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

case class EndMarker(symbol: Symbol)

object EndMarker:
val endMarkerRegex = """.*end(/\*.*\*/|\s)+""".r
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val endMarkerRegex = """.*end(/\*.*\*/|\s)+""".r
private val endMarkerRegex = """.*end(/\*.*\*/|\s)+""".r

and maybe an example of what it matches?

object EndMarker/*example.EndMarker.*/:
def foo/*example.EndMarker.foo().*/ =
1
end foo
Copy link
Contributor

Choose a reason for hiding this comment

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

They are mostly needed for definition so that should be fine.

tgodzik added a commit to scala/scala3 that referenced this pull request Nov 6, 2023
@kasiaMarek kasiaMarek force-pushed the pc-end-marker-rename branch from d9a926d to 14ca795 Compare November 7, 2023 10:45
@kasiaMarek kasiaMarek merged commit f46ca8c into scalameta:main Nov 8, 2023
24 checks passed
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.

Renaming of local functions misses end marker
4 participants