Import selector respects backquotes [ci: last-only] #6218

som-snytt · 2017-12-07T17:57:21Z

An import selector from WILDCARD to WILDCARD is taken as importing member underscore, and WILDCARD to anything else not null as renaming it. Renaming unquoted wildcard is not allowed.

was:
To disambiguate backquoted underscore, the convention
for ImportSelector is tweaked by using EMPTY instead of
WILDCARD as the special value. So EMPTY => EMPTY is
wildcard import, nonempty => EMPTY is hidden.

This encoding also avoids a null value for the rename field.

A rename is detected more simply by name != rename.

This change would require communication with anyone who builds ImportSelector manually. It would be nice to use ImportSelector.wild instead of relying on the encoding. See the test that does that.

Fixes scala/bug#6426
Fixes scala/bug#10641
Fixes scala/bug#7691

som-snytt · 2017-12-08T03:06:13Z

A couple of fixes for reflection. I was inclined to add API for ImportSelector such as isWildcard and isHidden. The conventional construction is distributed far and wide, for testing and making wildcards etc. Note that underscore remains _ in WildcardSelectorRepr and friends, because pq"_".

dwijnand · 2017-12-11T14:21:05Z

Btw, you need to say "fixes xx/yy#zz" for each xx/yy#zz, otherwise it'll just close the first one.

som-snytt · 2017-12-12T00:04:10Z

Also fix ImportInto.isExplicitImport by adding that API to ImportSelector.

retronym · 2018-01-12T04:56:26Z

I'd be most worried about compatibility with Scala IDE and Scala refactoring.

⚡ git grep WILDCARD | grep -i import
src/main/scala/scala/tools/refactoring/analysis/ImportAnalysis.scala:      imp.expr.toString() + "." + nme.WILDCARD.decoded
src/main/scala/scala/tools/refactoring/analysis/ImportAnalysis.scala:      val symImpStr = (nme.WILDCARD.decoded :: parts).reverse.mkString(".")
src/main/scala/scala/tools/refactoring/analysis/ImportAnalysis.scala:      if (sel.name == nme.WILDCARD)
src/main/scala/scala/tools/refactoring/analysis/ImportsToolbox.scala:        if (sel.name == nme.WILDCARD) List(mkName(thatQual))
src/main/scala/scala/tools/refactoring/implementations/ImportsHelper.scala:                          selector.name == nme.WILDCARD || {
src/main/scala/scala/tools/refactoring/implementations/UnusedImportsFinder.scala:  def wildcardImport(i: ImportSelector) = i.name == nme.WILDCARD
src/main/scala/scala/tools/refactoring/implementations/UnusedImportsFinder.scala:    if (s.rename == nme.WILDCARD) return true
src/main/scala/scala/tools/refactoring/implementations/oimports/ImportParticipants.scala:        importSelector.rename == nme.WILDCARD
src/main/scala/scala/tools/refactoring/implementations/oimports/ImportParticipants.scala:      importSelector.name == nme.WILDCARD || (allTypeNamesFromSelect(select) & allTypeNamesFromImportSelector(importSelector)).nonEmpty
src/main/scala/scala/tools/refactoring/implementations/oimports/ImportParticipants.scala:      val wild = imp.selectors.find(_.name == nme.WILDCARD)
src/main/scala/scala/tools/refactoring/implementations/oimports/ImportParticipants.scala:        val (wild, rest) = imports.partition(_.selectors.exists(_.name == nme.WILDCARD))
src/main/scala/scala/tools/refactoring/implementations/oimports/ImportParticipants.scala:            Some(imp.copy(qual, List(ImportSelector(nme.WILDCARD, -1, nme.WILDCARD, -1))).copyAttrs(imp))
src/main/scala/scala/tools/refactoring/implementations/oimports/RegionTransformations.scala:        importName.endsWith(selector.name.decoded) && selector.rename == nme.WILDCARD
src/main/scala/scala/tools/refactoring/implementations/oimports/RegionTransformations.scala:          selector.name == nme.WILDCARD || importsNames.exists { importName =>
src/main/scala/scala/tools/refactoring/implementations/oimports/RegionTransformations.scala:              case imp @ RegionImport(expr, sels) if sels.exists { _.name == nme.WILDCARD } =>
src/main/scala/scala/tools/refactoring/implementations/oimports/TreeToolbox.scala:              rsels.exists { _.name == global.nme.WILDCARD } && (treeComparables.isSameWithSymbols(true)(expr.symbol, rexpr.symbol) || treeComparables.isSameExprByName(true)(expr, rexpr)) ||

I wonder if we can find a way to leave the encoding as is, but sneak an extra boolean onto ImportSelector to identify the problematic (and esoteric) case of import foo.{_ => bar}. (Are there more problematic cases than that?)

som-snytt · 2018-01-12T08:12:07Z

Yes, for sure, I meant tooling needs communication, but that it would nicer if there were an API to make the change easier and more uniform, too. I could experiment on refactoring.

I think my first thought was a tree attachment.

dwijnand · 2018-06-06T22:15:37Z

Refactoring requested and new commits pushed. Re-review by @retronym, perhaps?

adriaanm · 2018-08-03T15:20:17Z

I like using EMPTY for the target of a rename that intends to hide a name (a => _), but don't understand why we can't represent a final _ import internally as (_ => _). It seems that would yield a smaller delta in the internal representation.

The caution about breaking tooling remains, but I'm fine moving forward as long as scalafix etc is not affected. (Which seems unlikely since they reimplemented all this ;-))

adriaanm · 2018-08-03T15:20:57Z

src/reflect/scala/reflect/api/Trees.scala

   *
   *  Would be represented as:
   *
-   *    Import(qual, List(("x", "x"), ("y", "z"), (WILDCARD, null)))
+   *    Import(qual, List(("w", EMPTY), ("x", "x"), ("y", "z"), (EMPTY, EMPTY)))


As mentioned in the discussion, could we use (WILDCARD, WILDCARD) for the last element?

Now (WILD, WILD) means import backticked underscore, possibly with rename. Otherwise, the encoding is unchanged.

SethTisue · 2018-11-06T13:44:38Z

@som-snytt do you plan to resume work on this?

src/compiler/scala/tools/nsc/typechecker/Namers.scala

som-snytt · 2018-11-26T23:37:46Z

@adriaanm This is totally incrementable and useful and unoffensive and could be moved up to 2.13.0 cycle. I was looking at imports today thinking I thought I'd changed that code.

adriaanm

💯 the code is much, much nicer! encapsulation FTW
🏅 for moxie
🃏 because wild card

This project now compiles with Scala 2.13 by default, while still cross-compiling to Scala 2.12. Issues addressed with this upgrade: ### Underscore (`_`) is no longer a valid identifer for vals Using underscore like this is no longer allowed... ``` implicit val _ = ... ``` ...we have to give the `val` a valid name: ``` implicit val v = ... ``` See also: * scala/bug#10384 * scala/bug#7691 * scala/scala#6218 ### `Map.mapValues()` now returns a `MapView` rather than a `Map` We usually want a `Map`, so we can just add in a `.toMap` to the end of the expression - it's harmless in Scala 2.12, and allows the code to compile in Scala 2.13. `Map.mapValues()` itself is deprecated in Scala 2.13, but using the new recommended alternative doesn't work in Scala 2.12. https://docs.scala-lang.org/overviews/core/collections-migration-213.html#what-are-the-breaking-changes See also: * scala/bug#10919 * scala/scala#7014

This project now compiles with Scala 2.13 by default, while still cross-compiling to Scala 2.12. Issues addressed with this upgrade: ### Underscore (`_`) is no longer a valid identifer for `val`s Using underscore like this is no longer allowed... ``` implicit val _ = ... ``` ...we have to give the `val` a valid name (even if it's just `v`): ``` implicit val v = ... ``` See also: * scala/bug#10384 * scala/bug#7691 * scala/scala#6218 ### `Map.mapValues()` now returns a `MapView` rather than a `Map` We usually want a `Map`, so we can just add in a `.toMap` to the end of the expression - it's harmless in Scala 2.12, and allows the code to compile in Scala 2.13. `Map.mapValues()` itself is deprecated in Scala 2.13, but using the new recommended alternative doesn't work in Scala 2.12. https://docs.scala-lang.org/overviews/core/collections-migration-213.html#what-are-the-breaking-changes See also: * scala/bug#10919 * scala/scala#7014 ### `.to` can no longer infer what resulting collection type you want This is all part of `CanBuildFrom` going away in Scala 2.13: https://www.scala-lang.org/blog/2018/06/13/scala-213-collections.html#life-without-canbuildfrom

This project now compiles with Scala 2.13 by default, while still cross-compiling to Scala 2.12 & 2.11. Issues addressed with this upgrade: ### Underscore (`_`) is no longer a valid identifer for `val`s Using underscore like this is no longer allowed... ``` implicit val _ = ... ``` ...we have to give the `val` a valid name (even if it's just `v`): ``` implicit val v = ... ``` See also: * scala/bug#10384 * scala/bug#7691 * scala/scala#6218 ### `Map.mapValues()` now returns a `MapView` rather than a `Map` We usually want a `Map`, so we can just add in a `.toMap` to the end of the expression - it's harmless in Scala 2.12, and allows the code to compile in Scala 2.13. `Map.mapValues()` itself is deprecated in Scala 2.13, but using the new recommended alternative doesn't work in Scala 2.12. https://docs.scala-lang.org/overviews/core/collections-migration-213.html#what-are-the-breaking-changes See also: * scala/bug#10919 * scala/scala#7014 ### `.to` can no longer infer what resulting collection type you want This is all part of `CanBuildFrom` going away in Scala 2.13: https://www.scala-lang.org/blog/2018/06/13/scala-213-collections.html#life-without-canbuildfrom

This project now compiles with Scala 2.13 by default, while still cross-compiling to Scala 2.12 & 2.11. Although Scala 2.13.1 has been released, this change sticks with 2.13.0, as `scoverage` has not yet released an update compatible with 2.13.1: scoverage/sbt-scoverage#295 scoverage/scalac-scoverage-plugin#283 scoverage/sbt-scoverage#299 Migration issues addressed with the update to Scala 2.13: ### Underscore (`_`) is no longer a valid identifer for `val`s Using underscore like this is no longer allowed... ``` implicit val _ = ... ``` ...we have to give the `val` a valid name (even if it's just `v`): ``` implicit val v = ... ``` See also: * scala/bug#10384 * scala/bug#7691 * scala/scala#6218 ### `Map.mapValues()` now returns a `MapView` rather than a `Map` We usually want a `Map`, so we can just add in a `.toMap` to the end of the expression - it's harmless in Scala 2.12, and allows the code to compile in Scala 2.13. `Map.mapValues()` itself is deprecated in Scala 2.13, but using the new recommended alternative doesn't work in Scala 2.12. https://docs.scala-lang.org/overviews/core/collections-migration-213.html#what-are-the-breaking-changes See also: * scala/bug#10919 * scala/scala#7014 ### `.to` can no longer infer what resulting collection type you want This is all part of `CanBuildFrom` going away in Scala 2.13: https://www.scala-lang.org/blog/2018/06/13/scala-213-collections.html#life-without-canbuildfrom

SethTisue · 2020-02-06T19:30:54Z

implicit val _ = ... used to work and now doesn't. that's as intended, but it would be nice if the compiler issued a warning

eed3si9n · 2020-08-13T14:33:04Z

Thanks for fixing this. Maybe we could mention in https://github.com/scala/scala/releases/tag/v2.13.0 that this allows multiple dummy binding.

What do you think about the following edit?

  * Underscore is no longer a legal identifier unless backquoted ([bug\#10384](https://github.com/scala/bug/issues/10384))
+   * Multiple dummy bindings can now be used within a scope: `{ val _ = 3; val _ = 5; 7 }`
    * `val _ =` is now a pattern match
    * `implicit val _ =` is also now a pattern match which does not introduce an identifier and therefore does not add anything to implicit scope

SethTisue · 2020-08-13T19:14:47Z

@eed3si9n hmm... not against adding some wording, but I think we should make it very clear to the reader why we're telling them this. (which the existing text isn't necessarily doing an ideal job of, either)

what is the recommended way to value-discard, warning-free? is it (...: Unit), or val _ = ...? I'm not sure which I like better, style-wise.

eed3si9n · 2020-08-13T19:23:01Z

Between the two, val _ = .... I think is less surprising to the person reading the code:

val _ = compile.value
val _ = copyResources.value

(...: Unit) has similar vibe to final val in a sense that all the tokens make sense, but the intent is opaque unless you're in the close circle of people who's been watching the development of 2.13 closely.

som-snytt · 2020-08-13T20:46:35Z

what is the recommended way to value-discard, warning-free?

@nowarn or -Wconf

"Warn-free, as free as the wind blows..." 🎶

SethTisue · 2020-08-13T21:57:26Z

okay, I changed that section of the notes to:

Underscore is no longer a legal identifier unless backquoted (bug#10384)
- val _ = is now a pattern match (and discards the value without incurring a warning)
- implicit val _ = is also now a pattern match (and is useless, because it no longer adds to implicit scope)

(The fact that as an additional consequence it is now allowed to val _ = ... multiple times seems like unnecessary extra detail to me.)

som-snytt · 2020-08-13T22:49:35Z

Also, from now on, we should call them the release 🎶 or is that already the name of Seth's band?

dwijnand · 2020-08-14T07:23:35Z

what is the recommended way to value-discard, warning-free? is it (...: Unit), or val _ = ...? I'm not sure which I like better, style-wise.

#7563 makes (...: Unit) the official way to avoid warnings.

scala-jenkins added this to the 2.13.0-M3 milestone Dec 7, 2017

som-snytt force-pushed the issue/underscore branch from 9a264d8 to eb0a231 Compare December 7, 2017 20:59

som-snytt force-pushed the issue/underscore branch 2 times, most recently from faae4c8 to 0ea6c72 Compare December 9, 2017 01:09

retronym self-requested a review January 12, 2018 04:41

retronym self-assigned this Jan 12, 2018

This comment has been minimized.

Sign in to view

adriaanm modified the milestones: 2.13.0-M3, 2.13.0-M4 Jan 12, 2018

This comment has been minimized.

Sign in to view

som-snytt force-pushed the issue/underscore branch from d76b832 to e7ac907 Compare April 25, 2018 19:52

This comment has been minimized.

Sign in to view

adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018

adriaanm suggested changes Aug 3, 2018

View reviewed changes

SethTisue modified the milestones: 2.13.0-M5, 2.13.0-RC1 Aug 7, 2018

adriaanm modified the milestones: 2.13.0-RC1, 2.13.x Aug 7, 2018

SethTisue added the WIP label Nov 6, 2018

som-snytt commented Nov 6, 2018

View reviewed changes

src/compiler/scala/tools/nsc/typechecker/Namers.scala Outdated Show resolved Hide resolved

adriaanm modified the milestones: 2.13.1, 2.13.0-RC1 Nov 27, 2018

adriaanm approved these changes Nov 27, 2018

View reviewed changes

adriaanm merged commit 1bf19cd into scala:2.13.x Nov 27, 2018

som-snytt deleted the issue/underscore branch November 27, 2018 12:11

SethTisue mentioned this pull request Dec 4, 2018

2.13: move to new collections scala/community-build#710

Closed

som-snytt mentioned this pull request Dec 16, 2018

Introduce Unit.void[A](x: A): Unit #7530

Closed

3 tasks

xuwei-k mentioned this pull request Jan 22, 2019

avoid underscore as identifier for Scala 2.13.x ? com-lihaoyi/fastparse#214

Closed

som-snytt mentioned this pull request Mar 4, 2019

Improve import selector parsing scala/scala3#6019

Closed

diesalbla mentioned this pull request Mar 6, 2019

Namers - no duplicates: backport changes to avoid allocations [nomerge] #7820

Merged

rtyley mentioned this pull request Oct 21, 2019

Add Scala 2.13 support DanielaSfregola/twitter4s#275

Merged

eed3si9n mentioned this pull request Aug 13, 2020

spurious "a pure expression does nothing" warning (awkward to suppress in 2.12, prone to occur in sbt builds) scala/bug#12112

Open

som-snytt mentioned this pull request Nov 5, 2020

Fix #10178: Put the wildcard demon back in the bottle scala/scala3#10195

Merged

som-snytt mentioned this pull request Mar 22, 2022

Accessing backqouted underscore does not parse but compiles fine in Scala 2.13.x scala/scala3#14746

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import selector respects backquotes [ci: last-only] #6218

Import selector respects backquotes [ci: last-only] #6218

som-snytt commented Dec 7, 2017 •

edited

Loading

som-snytt commented Dec 8, 2017 •

edited

Loading

dwijnand commented Dec 11, 2017

som-snytt commented Dec 12, 2017

retronym commented Jan 12, 2018

som-snytt commented Jan 12, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dwijnand commented Jun 6, 2018

adriaanm commented Aug 3, 2018

adriaanm Aug 3, 2018

som-snytt Nov 9, 2018

SethTisue commented Nov 6, 2018

som-snytt commented Nov 26, 2018

adriaanm left a comment

SethTisue commented Feb 6, 2020 •

edited

Loading

eed3si9n commented Aug 13, 2020

SethTisue commented Aug 13, 2020

eed3si9n commented Aug 13, 2020

som-snytt commented Aug 13, 2020

SethTisue commented Aug 13, 2020 •

edited

Loading

som-snytt commented Aug 13, 2020

dwijnand commented Aug 14, 2020

Import selector respects backquotes [ci: last-only] #6218

Import selector respects backquotes [ci: last-only] #6218

Conversation

som-snytt commented Dec 7, 2017 • edited Loading

som-snytt commented Dec 8, 2017 • edited Loading

dwijnand commented Dec 11, 2017

som-snytt commented Dec 12, 2017

retronym commented Jan 12, 2018

som-snytt commented Jan 12, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dwijnand commented Jun 6, 2018

adriaanm commented Aug 3, 2018

adriaanm Aug 3, 2018

Choose a reason for hiding this comment

som-snytt Nov 9, 2018

Choose a reason for hiding this comment

SethTisue commented Nov 6, 2018

som-snytt commented Nov 26, 2018

adriaanm left a comment

Choose a reason for hiding this comment

SethTisue commented Feb 6, 2020 • edited Loading

eed3si9n commented Aug 13, 2020

SethTisue commented Aug 13, 2020

eed3si9n commented Aug 13, 2020

som-snytt commented Aug 13, 2020

SethTisue commented Aug 13, 2020 • edited Loading

som-snytt commented Aug 13, 2020

dwijnand commented Aug 14, 2020

som-snytt commented Dec 7, 2017 •

edited

Loading

som-snytt commented Dec 8, 2017 •

edited

Loading

SethTisue commented Feb 6, 2020 •

edited

Loading

SethTisue commented Aug 13, 2020 •

edited

Loading