Skip to content

improvement: use heuristic to figure out nameSpan if pointDelta too big #22484

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

Merged
merged 2 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions compiler/src/dotty/tools/dotc/ast/Trees.scala
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,11 @@ object Trees {
else if qualifier.span.exists && qualifier.span.start > span.point then // right associative
val realName = name.stripModuleClassSuffix.lastPart
Span(span.start, span.start + realName.length, point)
else
Span(point, span.end, point)
else if span.pointMayBeIncorrect then
val realName = name.stripModuleClassSuffix.lastPart
val probablyPoint = span.end - realName.length
Span(probablyPoint, span.end, probablyPoint)
else Span(point, span.end, point)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't even see where select gets its point; I see the span is set in typedSelectOnTerm for example. I haven't looked at the span computations there. But, it seems to me I can never trust the point of a huge span, and that counting back from the end is always correct in that case. Or, is there logic somewhere that overflowing point is reset to zero?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, overflowing is set to zero. The exact line is in the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, my eyes saw it but my brain did not.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this particular case, the span is set in selectorOrMatch in Parsers.scala by atSpan method.

else span
}

Expand Down
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/util/Spans.scala
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ object Spans {
if (poff == SyntheticPointDelta) start else start + poff
}

def pointMayBeIncorrect =
pointDelta == 0 && end - start >= SyntheticPointDelta
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it dubious to make this API of span? Why this check on pointDelta if any large span may make it overflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

If pointDelta it too large, pointDelta will be set to 0, so only then it can be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I withdraw my objection to adding the method to Span. As a footnote, there is no range check on point, so "incorrect" code can attempt to assign a point "outside" the span. This was an ongoing issue in Scala 2. I haven't looked again yet at the "multiple vars" position issue, but that is the sort of incorrect code I mean.


/** The difference between point and start in this span */
def pointDelta: Int =
(coords >>> (StartEndBits * 2)).toInt
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1462,5 +1462,106 @@ class DocumentHighlightSuite extends BaseDocumentHighlightSuite:
|""".stripMargin
)

@Test def i3053 =
check(
"""import Aaaa.*
|
|def classDef2(cdef: List[Int]): Int = {
| def aaa(ddef: Thicket2): List[Int] = ddef match {
| case Thicket2(_) => ???
| }
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we skip the scaladoc actually, or make them a bit shorter?

Copy link
Member Author

@kasiaMarek kasiaMarek Feb 3, 2025

Choose a reason for hiding this comment

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

The problem is that the span is too big, so all these comments to make the method extra long are needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ach, makes sense. Thanks for explaining!

Copy link
Contributor

Choose a reason for hiding this comment

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

It definitely falls under "things I would not have anticipated"!

| * inline classes
| */
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
| * inline classes
| */
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
| * inline classes
| */
| // Annotations on class _type_ parameters are set on the derived parameters
| // but not on the constructor parameters. The reverse is true for
| // annotations on class _value_ parameters.
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
| * inline classes
| */
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
| * inline classes
| */
|
| // Annotations on class _type_ parameters are set on the derived parameters
| // but not on the constructor parameters. The reverse is true for
| // annotations on class _value_ parameters.
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
| * inline classes
| */
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
|
| /** Does `tree' look like a reference to AnyVal? Temporary test before we have
| * inline classes
| */
| // Annotations on class _type_ parameters are set on the derived parameters
| // but not on the constructor parameters. The reverse is true for
| // annotations on class _value_ parameters.
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
| // The original type and value parameters in the constructor already have the flags
| // needed to be type members (i.e. param, and possibly also private and local unless
| // prefixed by type or val). `tparams` and `vparamss` are the type parameters that
| // go in `constr`, the constructor after desugaring.
| 1
|}.<<showing2>>("aaa")
Copy link
Contributor

Choose a reason for hiding this comment

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

The > 4K string could be generated "x" * 4096 instead of adding text to the repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kasiaMarek Let's maybe to that and then merge?

|
|case class Thicket2(trees: List[Int]) {}
|
|object Aaaa {
| extension [T](x: T)
| def <<show@@ing2>>[U](aaa: String): T = {
| x
| }
|}
|
|""".stripMargin
)


end DocumentHighlightSuite
Loading