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

Fix selecting terms using _root_ #18335

Merged
merged 4 commits into from
Aug 14, 2023
Merged

Fix selecting terms using _root_ #18335

merged 4 commits into from
Aug 14, 2023

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Aug 3, 2023

No description provided.

@dwijnand dwijnand linked an issue Aug 3, 2023 that may be closed by this pull request
@dwijnand dwijnand marked this pull request as ready for review August 7, 2023 07:52
@dwijnand dwijnand requested a review from odersky August 7, 2023 07:52
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I find this deeply inelegant. Checking for _root_ when parsing every ident is terrible.

We should disallow definitions of _root_ in valdefs or defdefs instead. That's seems like the cleaner fix to me.

@odersky
Copy link
Contributor

odersky commented Aug 7, 2023

Or do nothing and let the chips fall, which was the state before. I believe it is much preferable to just leave _root_ as a regular identifier and live with malicious code that defines alternates than to mess up the parser like this.

That's why it was like this in the first place. The suspicion was that it's not a bug worth fixing, i.e. the fix would likely be worse than the bug. The change in this PR confirms that suspicion for me.

So if we do anything, we can try to rule out definitions. Or revert to the status quo before we decided to change things with _root_.

Switch to only reporting val and def names
@som-snytt
Copy link
Contributor

Ginseng is a select root.

Scala 2 does the opposite and merely lints the usage, placing no additional restrictions. It's only a backstop.

scala> object x { object _root_ { def y = 42 } }; x._root_.y
object x
val res0: Int = 42

scala> object x { object _root_ { def y = 42 }; def z = _root_.y }; x._root_.y
                                                               ^
       error: object y is not a member of package <root>
                                                        ^
       warning: _root_ in root position of qualifier refers to the root package, not object _root_ in object x, which is in scope

scala>

@odersky odersky assigned dwijnand and unassigned odersky Aug 7, 2023
@dwijnand dwijnand requested a review from odersky August 7, 2023 18:14
@dwijnand dwijnand assigned odersky and unassigned dwijnand Aug 7, 2023
@dwijnand
Copy link
Member Author

dwijnand commented Aug 7, 2023

We only check val/def definitions now.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I thought we could not burden Parser with this, but check later at Typer? is there a reason not to do this?

@odersky odersky assigned dwijnand and unassigned odersky Aug 7, 2023
@dwijnand
Copy link
Member Author

dwijnand commented Aug 8, 2023

I thought we could not burden Parser with this, but check later at Typer? is there a reason not to do this?

It seemed like logic that is simple enough it could be done in Parser. I'll have a look at what it looks like done in Typer.

tests/neg/i18020.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
@odersky odersky assigned dwijnand and unassigned dwijnand Aug 8, 2023
@som-snytt
Copy link
Contributor

Comment from ten years ago (as I was spelunking today):

@adriaanm said:
Optimization == evil.root

I propose we update the pun to _root_.evil.

That may also be theologically more sound.

@som-snytt
Copy link
Contributor

Tests failures of the form

 Test 'tests/run-staging/i3876-b.scala' failed with output:                      

  exception while retyping package _root_ {
  class Generated$Code$From$Quoted() extends Object() {
    def apply: Any =
      {
        def f(x: Int): Int = x.+(x)
        f(3)
      }
  }
} of class PackageDef # -1

but I'm rooting for you!

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Interesting that we need that for reflection. But yes, makes sense.

@dwijnand dwijnand merged commit 2be692d into scala:main Aug 14, 2023
@dwijnand dwijnand deleted the select-root branch August 14, 2023 20:36
@Kordyjan
Copy link
Contributor

I assume it is a follow-up to #18187, so leaving it out of lts unless there is significant interest.

@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
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.

Regression in armanbilge/gcp4s, unallow _root_ selector
4 participants