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

Promote -deprecation to -Xlint:deprecation #7714

Merged
merged 1 commit into from
Mar 22, 2019

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Feb 4, 2019

Promote -deprecation to -Xlint:deprecation, and demote complaining about missing since to lint.

(Setting the lint option also enables ordinary -deprecation; the lint is a superset.)

Was: Adopt forRemoval flag for deprecated.

if (isJava) annType.decls.filter(sym => sym.isMethod && !sym.isConstructor && sym.isJavaDefined)
else EmptyScope // annScopeJava is only used if isJava

val names = mutable.Set[Symbol]()
Copy link
Contributor

Choose a reason for hiding this comment

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

The class Symbol includes an instance-unique id value, of type Int. Instead of using a Set of symbols, could we use a set of their ids? Sets of integers have efficient implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A sage conjecture.

@eed3si9n
Copy link
Member

JEP 277: Enhanced Deprecation says

However, the @Deprecated annotation ended up being used for several different purposes. Very few deprecated APIs were actually removed, leading some people to believe that nothing would ever be removed. On the other hand, other people believed that everything that was deprecated might eventually be removed, which was never the intent either. (Although it wasn't stated explicitly in the specifications, various documents mentioned that deprecated APIs would be removed at some point.) This resulted in an unclear message being delivered to developers about the meaning of @Deprecated, and what, if anything, developers should do when they encountered usage of a deprecated API. Everybody was confused about what deprecation actually meant, and nobody took it seriously. This in turn has made it difficult ever to remove anything from the Java SE API.

How did Java programmers confuse themselves to the point that they don't know what @Deprecated mean? Does this confusion exist among the Scala programmers? At least for me, @deprecated means that the API is eventually going away. Adding forRemoval I feel like would actually increase the uncertainty rather than clarity.

If some API says @deprecated("Don't use it.", since="1.0") in Scala 2.13, what would that mean? I would much rather go the opposite direction of adding @restricted or whatever for non-removal warnings like #7790.

@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 20, 2019

@eed3si9n Although I have some opinions about deprecation and migration which I won't bother to elaborate, I will offer unequivocally that deprecation is not just about removing API.

For example, "It encourages bad coding practices", https://docs.oracle.com/javase/1.5.0/docs/guide/javadoc/deprecation/deprecation.html, is a possible semantics.

You could deprecate appending to a List with no intention of removing the API.

Someone, maybe smarter, said Scala is different from Java because it throws out API all the time.

But even Scala would not throw out list append. There is no mechanism for doing so. But it could deprecate append when the receiver is statically known to be a List. The message might suggest ListBuffer, or prepend and reverse, or what have you.

Deprecation is a way to ward off impending doom. It is a prayer which says to the gods, But I am too unimportant to destroy just now, let me live a bit longer as you purge the evil-doers. It is always unclear how long such a dispensation may last.

Edit: I mean to describe deprecation as understood, not to suggest that Scala couldn't rewrite the rules, though in that case I'd probably suggest discarding the word "deprecation".

@som-snytt
Copy link
Contributor Author

I just read Shakespeare's Tempest. @NthPortal is like Ariel, swooping in to confuse us by liking each contradictory opinion.

You know it was only an illusion that the ship split apart in the storm, which itself was also an illusion.

"Their rising senses begin to chase the ignorant fumes that mantle their clearer reason."

@NthPortal
Copy link
Contributor

@som-snytt lol, I just thought both of you made good points.

Honestly though, I would prefer a different annotation for the List example you gave, because it's still part of the API, and if you need to append a single value to a List (which is conceivable), it's what you should use. In my mind, deprecated means "don't use this anymore", not "well this is generally not what you want, but sometimes it is, so you might not want to use it, unless you actually do".

@adriaanm
Copy link
Contributor

I'm reminded of our notorious greek cliff-avoidance comment in the compiler. Sadly, it doesn't look like we'll get this ship/boat/better-deprecation-story to the 2.13 port.

@adriaanm
Copy link
Contributor

As I said on the other PR -- I'd really-really-really like to get something like this into 2.14, since we can't get there for 2.13 it seems. Closing this one until we branch for 2.14 😞

@adriaanm adriaanm closed this Mar 21, 2019
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Mar 21, 2019
@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 21, 2019

Reopening to revert just the surprisingly controversial parameter.

Restored to RC1 because of the lint option.

@som-snytt som-snytt reopened this Mar 21, 2019
@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Mar 21, 2019
@som-snytt som-snytt changed the title Deprecated forRemoval Promote -deprecation to -Xlint:deprecation Mar 21, 2019
@som-snytt som-snytt modified the milestones: 2.13.1, 2.13.0-RC1 Mar 21, 2019
Adopt `forRemoval` flag for `deprecated`.
Revert forRemoval.

Promote `-deprecation` to `-Xlint:deprecation`,
and demote complaining about missing `since` to lint.

Beautify test for annot defaults.
Refactor typedAnnot to reveal structure.
Also reduce indentation.
@adriaanm adriaanm merged commit c7ef488 into scala:2.13.x Mar 22, 2019
@adriaanm
Copy link
Contributor

Maybe we can sneak in forRemoval using hashtags. message="don't use it #forRemoval", or when less serious, message="don't use it #justKidding".

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Mar 22, 2019
@som-snytt som-snytt deleted the topic/for-removal branch March 22, 2019 15:32
@som-snytt
Copy link
Contributor Author

@NthPortal footnote, the constructors for java boxed primitives says: It is rarely appropriate to use this constructor. They tease us with "rarely", but it would be an example where "don't use this" is qualified.

finaglehelper pushed a commit to twitter/finatra that referenced this pull request Mar 11, 2021
Problem
=======

Upgrade source to https://github.com/scala/scala/releases/tag/v2.12.13 from https://github.com/scala/scala/releases/tag/v2.12.12. This upgrade includes a highlighted feature of configurable warnings and errors (https://www.scala-lang.org/2021/01/12/configuring-and-suppressing-warnings.html / scala/scala#9248).

Other noticable changes our code based will experience

* Exhaustivity warnings for patterns involving tuples
* Better type checking for `CharSequence` arguments scala/scala#9292
* Simplified Reporters scala/scala#8338
* Promotion of `-deprecation` to `-Xlint:deprecation` scala/scala#7714
* Improves performance of building `immutable.{TreeMap,TreeSet}` by using mutation within the builder scala/scala#8794

Full list of changes can be found at https://github.com/scala/scala/pulls?q=is%3Amerged+milestone%3A2.12.13+

Solution
========

* Bump `BUILD` file `SCALA_REV` && `SCALAC_REV_FOR_DEPS`.
* Depdnencies which are not built for scala 2.12.13 needed to be bumped `SEMANTICDB_PLUGIN_REV` && `SEMANTICDB_REV` && `SCALAFIX_REV`.
* Include `-S-Xlint:-deprecation` to `pants.ini` preventing build failures on deprecated annotations (existing behavior)
* Bump `cache_key_gen_version` in `pants.ini` for newly built artifacts on different scala version.
* Removed scalafix plugin (`scalac-profiling`) which is not built for 2.12.13. `scalac-profiling` code looks abandoned by ~3 years.
* Updated all failing tests that could have depended or expected ordered sequences when the sequence was generated from non ordered collections.

Notes
=====

It seems a few tests and golden files in source have depended or tested against a stable sort order when sorted order is not guaranteed. This has been seen in a few places such as output json objects (map key fields), code that uses `groupBy` for rekeying results to the user and transforming into sequences, strings built from sequences or maps that are not guaranteed to be ordered.

The following PR are related to this change in expectations

scala/scala#8794
scala/scala#8948
scala/scala#9218
scala/scala#9376
scala/scala#9091
scala/scala#9216

We took the liberty updating tests with what the current map order would be or updating the test in a way that wouldn't depend on order. Since we may not fully understand all the context of the tests we are hoping this either signals to the owners that there might be some issue with assumed order or signal that upgrading might break implementations due to bug in scala 2.12.13. {D611202} will improve the files changed for workflow builder outside of this change.

Please feel to reach out directly and discuss the changes here or bring up issues with this upgrade. Slack [[https://app.slack.com/client/T86S8GHEG/C01NZAFRLFK|#scala-upgrade-2-12-13]]

JIRA Issues: SCALA-25

Differential Revision: https://phabricator.twitter.biz/D607826
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
7 participants