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

Deprecate symbol literals #7395

Merged
merged 1 commit into from
Feb 9, 2019

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Nov 6, 2018

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Nov 6, 2018
@eed3si9n
Copy link
Member Author

eed3si9n commented Nov 6, 2018

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Nov 6, 2018
@SethTisue
Copy link
Member

I see that you made it so that under -Xsource:2.14, it's an error 👍

@SethTisue
Copy link
Member

last call for further discussion at scala/scala-dev#459 . my sense from the discussion there is that we are likely to merge this.

@ashawley
Copy link
Member

ashawley commented Feb 1, 2019

Seems like this should move forward given the discussion so far on dropping symbol literals. There's a preponderance of evidence that this can work, so it seems marking symbol literals as deprecated can move forward.

I investigated the subsequent step of experimentally dropping symbol literals entirely. I dropped them in 2.13 and backported it to 2.12. I also used a shorthand syntax for symbols in #7395, rather than Symbol.apply, throughout the compiler and the standard library. It went smoothly.

I did the exercise on 2.12 so that I could see the impact on community libraries. At the time, the 2.13 community build was in too much flux. With @SethTisue's assistance, I was able drop symbol literals in 24 community projects that had literal symbols in their build, see scala/community-build#827. I added the new symbol syntax to these builds, and this resulted in 92 projects built successfully with a 2.12 compiler with symbol literals dropped.

There was one project of the 24 that the new symbol syntax could not be made to work, Shapeless, but there's evidence that it could be made to work with Symbol.apply. Alternatively, Shapeless could undergo some significant re-architecting to shorthand symbols or could drop symbol support entirely.

There were 85 builds that didn't run in the 2.12 community build because a dependency failed to build or because Shapeless can't build. I stopped going further only for lack of time.

@SethTisue
Copy link
Member

@eed3si9n are you available to rebase this and fix CI for merge?

@eed3si9n
Copy link
Member Author

eed3si9n commented Feb 5, 2019

@SethTisue Rebased against master.

@SethTisue
Copy link
Member

@eed3si9n CI logs show that partests need updating. note that if you rebase again you'll get #7495 which will let you use sym"foo" if you want to

@ashawley
Copy link
Member

ashawley commented Feb 6, 2019

We're only deprecating symbol literals, so there's no need to modify tests by replacing them with Symbol or even sym.† We can look forward to doing that, or even deleting many of the tests, when symbol literals are dropped in 2.14.

I'd argue that the tests should just be updated from the deprecation. I think this will do that:

partest --update-check test/files/neg/checksensible.scala test/files/neg/names-defaults-neg.scala test/files/run/t10646.scala test/files/run/t4560.scala test/files/run/t4601.scala test/files/run/t6632.scala test/files/run/t6633.scala test/files/run/t6634.scala test/files/run/t7974 test/files/run/t8933 test/files/run/t8933b test/files/jvm/manifests-new.scala test/files/jvm/manifests-old.scala test/files/jvm/serialization-new.scala test/files/jvm/serialization.scala test/files/run/SymbolsTest.scala test/files/run/arrays.scala test/files/run/fors.scala test/files/run/lisp.scala

† I appreciate the sym evangelism. FWIW, I did a single test of the sym syntax throughout the compiler in #7499 and then in the community build. What I learned from that exercise I added to a JUnit test for sym.

@eed3si9n
Copy link
Member Author

eed3si9n commented Feb 6, 2019

@ashawley Thanks! That was helpful.

@ashawley
Copy link
Member

ashawley commented Feb 6, 2019

After changing the deprecation message, you just need to update your partests:

partest --update-check test/files/neg/symbol-literal-deprecation.scala test/files/neg/symbol-literal-removal.scala

@ashawley
Copy link
Member

ashawley commented Feb 6, 2019

If you have it in you, you could roll back the changes to the old quoted symbol tests:

git checkout 2.13.x -- test/files/run/SymbolsTest.scala test/files/run/arrays.scala test/files/run/fors.scala test/files/run/lisp.scala test/files/run/t10646.scala test/files/run/t8933/A_1.scala test/files/run/t8933/Test_2.scala test/files/run/t8933b/A.scala test/files/run/t8933b/Test.scala

And then update the tests:

partest --update-check test/files/run/SymbolsTest.scala test/files/run/arrays.scala test/files/run/fors.scala test/files/run/lisp.scala test/files/run/t10646.scala test/files/run/t8933 test/files/run/t8933b

@eed3si9n
Copy link
Member Author

eed3si9n commented Feb 6, 2019

@ashawley Are you saying you'd prefer the symbol literal kept as-is in the tests?
I think it's better to migrate the test as much as possible when we deprecate a feature to avoid future pile-up of old syntax.

@ashawley
Copy link
Member

ashawley commented Feb 6, 2019

I understand the urge would be to migrate them for the sake of reducing technical debt. That may be possible for certain types of deprecations, but here it would remove the code coverage of the parser for symbol literals in the 2.13 test suite for now and evermore. I'm not sure if there are typically a lot of changes to the parser in bug-fix releases of the compiler or not, but if so I'd want to keep those symbol literal tests as-is.

@SethTisue SethTisue merged commit f46fc7e into scala:2.13.x Feb 9, 2019
@eed3si9n eed3si9n deleted the wip/deprecate-symbol-literal branch February 9, 2019 21:46
@SethTisue SethTisue changed the title Deprecate symbol literal Deprecate symbol literals Apr 4, 2019
ioannakok added a commit to guardian/frontend that referenced this pull request Aug 11, 2022
In Scala 2.13 symbol literals are being deprecated: scala/scala#7395

The recommendation from Scala 2.10.0 notes (https://github.com/scala/scala/releases/v2.13.0) is:

```
Symbol literals deprecated
- Symbols themselves remain supported, only the single-quote syntax is deprecated. (scala/scala#7395)
- Library designers may wish to change their APIs to use String instead.
- Deprecated: 'foo
- Use instead: Symbol("foo")
```

The changes in this commit will help us to avoid errors like the following when switching to 2.13:

```
[error] /Users/ioanna_kokkini/Projects/frontend/admin/app/views/commercial/adsDotText.scala.html:20:43: symbol literal is deprecated; use Symbol("_label") instead
[error]         @textarea(sellersForm("sellers"), '_label -> "Authorised sellers:", 'rows -> 30, 'cols -> 100, '_help -> "")
```

Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk>
domlander pushed a commit to guardian/frontend that referenced this pull request Aug 11, 2022
In Scala 2.13 symbol literals are being deprecated: scala/scala#7395

The recommendation from Scala 2.10.0 notes (https://github.com/scala/scala/releases/v2.13.0) is:

```
Symbol literals deprecated
- Symbols themselves remain supported, only the single-quote syntax is deprecated. (scala/scala#7395)
- Library designers may wish to change their APIs to use String instead.
- Deprecated: 'foo
- Use instead: Symbol("foo")
```

The changes in this commit will help us to avoid errors like the following when switching to 2.13:

```
[error] /Users/ioanna_kokkini/Projects/frontend/admin/app/views/commercial/adsDotText.scala.html:20:43: symbol literal is deprecated; use Symbol("_label") instead
[error]         @textarea(sellersForm("sellers"), '_label -> "Authorised sellers:", 'rows -> 30, 'cols -> 100, '_help -> "")
```

Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants