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

Add -Yimports compiler flag #16218

Merged
merged 4 commits into from
Nov 26, 2022
Merged

Add -Yimports compiler flag #16218

merged 4 commits into from
Nov 26, 2022

Conversation

adampauls
Copy link
Contributor

@adampauls adampauls commented Oct 20, 2022

Fixes lampepfl/dotty-feature-requests#138. I was always unclear on the interaction between -Yimports and -Yno-imports -- are they additive or redundant -- so I made the name of the flag -Yadditional-imports so that its semantics are very clear.

EDIT: Okay, I kept the original (confusing IMHO) behavior of -Yimports from Scala 2.

I think this might be possibly controversial because it is a flag that technically affects typechecking, but I still think it would be quite handy to have.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Also, we should stick to how Scala 2 calls it, ie. -Yimports

@adampauls
Copy link
Contributor Author

adampauls commented Oct 24, 2022

I changed the name because it's really not clear how -Yno-imports and -Yimports foo interact. As in, I don't even know what the spec is, I think I'd have to look at the code to understand. It's possible that the old -Yimports was a set and not an add, in which case I think it's right to change the name. Put another way, if I had

class Foo(myImports: List[String]) {
  def noImports(): Foo = ???
  def imports(packages: List[String]): Foo = ???
}

What would you expect imports to do if you couldn't see the implementation? I don't think it would be to add them.

@adampauls
Copy link
Contributor Author

adampauls commented Oct 24, 2022

Hmm, if I'm reading the Scala 2 code right, -Yimports is a set and nopredef and -Yno-imports don't actually do anything? This is all I could find https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/typechecker/Contexts.scala#L159.

@adampauls
Copy link
Contributor Author

Apologies, it appears that the other import flags mutate the import settings https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/settings/ScalaSettings.scala#L195. So imports is a set and I think that's unfortunate because appending imports to the defaults becomes painful. However, if that's the preferred behavior, I'll change the behavior and name here.

@dwijnand
Copy link
Member

Yeah, we should match the behaviour, whatever it is. Even if we don't like it any more, changing it would create more confusion so should only be done for a very good reason.

@adampauls
Copy link
Contributor Author

I respectfully disagree -- it's not confusing when an old flag goes away and a new one with different behavior appears. But as it so happens, I'm mostly interested in the version where you would pair it with -Yno-imports anyways, so I suppose I shouldn't complain too much.

@adampauls adampauls changed the title Add -Yadditional-imports compiler flag Add -Yimports compiler flag Oct 25, 2022
@adampauls adampauls changed the base branch from main to release-3.2.1 October 25, 2022 04:12
@adampauls adampauls changed the base branch from release-3.2.1 to main October 25, 2022 04:12
@adampauls
Copy link
Contributor Author

Okay, I made the changes. I left the move over to comment-based CLI args as a separate commit.

@adampauls adampauls requested review from dwijnand and som-snytt and removed request for dwijnand and som-snytt October 25, 2022 04:13
@adampauls adampauls changed the base branch from main to release-3.2.1 October 25, 2022 05:26
@adampauls adampauls changed the base branch from release-3.2.1 to main October 25, 2022 05:26
@adampauls adampauls requested review from som-snytt and dwijnand and removed request for dwijnand and som-snytt October 25, 2022 21:56
@som-snytt
Copy link
Contributor

FWIW, scala 2 -Yimports defines the (ordered) root contexts as described in the (revised) spec https://scala-lang.org/files/archive/spec/2.13/02-identifiers-names-and-scopes.html

Ideally it would be -Xroot-contexts. There is history behind the existing name and behavior, and not everyone likes the behavior and would prefer a different feature for "preamble imports" or "preamble code".

There is precedent for renaming a compiler option but keeping the old name as an alias.

@adampauls adampauls requested review from dwijnand and removed request for som-snytt November 7, 2022 02:27
@adampauls
Copy link
Contributor Author

I'm happy with whatever will get approved!

@dwijnand dwijnand assigned dwijnand and unassigned adampauls Nov 7, 2022
@adampauls
Copy link
Contributor Author

Friendly bump on this PR?

@dwijnand
Copy link
Member

dwijnand commented Nov 21, 2022

Coincidentally ended up spending a long detour on imports in the REPL (#16389), which sets me up to be much better informed on imports. I'm reviewing this next. Sorry for the long delay 😞

@som-snytt
Copy link
Contributor

This definitely does not qualify as a long delay in this repository.

@dwijnand dwijnand merged commit 86e36e7 into scala:main Nov 26, 2022
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 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.

Support -Yimports for implicit preamble imports
4 participants