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

Improvements to deprecations related to `since` parameter #5076

Merged
merged 5 commits into from May 30, 2016

Conversation

Projects
None yet
5 participants
@soc
Copy link
Member

soc commented Apr 2, 2016

No description provided.

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 2, 2016

@soc soc force-pushed the soc:topic/deprecations-since branch 5 times, most recently from 60dd14a to 67421bc Apr 2, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Apr 3, 2016

Review by @SethTisue?

def deprecationMessage = getAnnotation(DeprecatedAttr) flatMap (_ stringArg 0)
def deprecationVersion = getAnnotation(DeprecatedAttr) flatMap (_ stringArg 1)
def deprecatedParamName = getAnnotation(DeprecatedNameAttr) flatMap (_ symbolArg 0 orElse Some(nme.NO_NAME))
def deprecatedParamVersion = getAnnotation(DeprecatedNameAttr) flatMap (_ stringArg 1)

This comment has been minimized.

@som-snytt

som-snytt Apr 3, 2016

Contributor

This longer name caused a big shift for column alignment, while subsequent long names are wrapped. Just eyeballing, it doesn't look determined by line length. Maybe avoid white space changes?

This comment has been minimized.

@soc

soc Apr 3, 2016

Author Member

I kept the wrapping starting with the next "category" of methods, it didn't feel right to have deprecatedParamName unwrapped and deprecatedParamVersion wrapped. If I wrapped deprecatedParamName I would also have to start wrapping the same-length hasBridgeAnnotation above...

This comment has been minimized.

@som-snytt

som-snytt Apr 3, 2016

Contributor

I would have wrapped deprecatedParamVersion; my eyes see a long block of stuff about deprecated stuff; I was only thinking of minimizing whitespace change so it's easier to see the real diff later. I don't know if github will ignore whitespace in diff views. Or maybe add vertical space if grouping is significant. Just a thought.

This comment has been minimized.

@soc

soc Apr 3, 2016

Author Member

?w=1

@@ -68,8 +68,9 @@ trait Reporting extends scala.reflect.internal.Reporting { self: ast.Positions w
// behold! the symbol that caused the deprecation warning (may not be deprecated itself)
def deprecationWarning(pos: Position, sym: Symbol, msg: String): Unit = _deprecationWarnings.warn(pos, msg)
def deprecationWarning(pos: Position, sym: Symbol): Unit = {
val suffix = sym.deprecationMessage match { case Some(msg) => ": "+ msg case _ => "" }
deprecationWarning(pos, sym, s"$sym${sym.locationString} is deprecated$suffix")

This comment has been minimized.

@som-snytt

som-snytt Apr 3, 2016

Contributor

I wonder if there is definite guidance on pattern matching vs map + getOrElse vs fold. I did ask about single line cases without semicolon, and no one liked it. (I liked the idea of it.)

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Apr 3, 2016

There is a JEP for improved deprecation in Java 9. I see it's linked on jira. They recently simplified, in order not to encode reasons and lifecycle disposition and replacement API that should be discussed in documentation. I can't tell if they eliminated "condemned" or renamed it "forRemoval"? But one point is that it doesn't give enough information, since projects have arbitrary lifecycles or policies for whether to remove deprecated API.

In a word, I'm not sure f has been deprecated (since 2.11) in an error message tells me more useful info. If there's more to be said about what deprecation means for this API, I should read the docs. Just because something has been deprecated since 2.9 doesn't mean it's scheduled for removal. That's certainly not the case for projects in general. So reporting it may be a job for a doc or lint tool.

Also, javac says "has been deprecated." For some reason it sounds more natural than "is deprecated." That may be because the verb is transitive or for sense of completion. "--What happened to the food? --The food has been eaten." Food in general is eaten because edible.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Apr 3, 2016

The whole reason why since is a String was that projects can add their own project name in front of it.

I know the Java 9 JEP. The problem is that they are still living in a world where the only imaginable way of dealing with deprecations is their own. From an improvement point of view, I think that @deprecated{,Name,Overriding,Inheritance} is vastly better for developers than their semantic games of adding more nuance to their deprecation annotations. I'm pretty pretty sure they will revert back to "we never remove anything deprecated" after they managed to ship Jigsaw anyway.

"deprecated (since FooBarLib 33.2)" is meaningful, because developers can check what the project's deprecation policy is.

That's the idea behind it: Instead of giving people "13 deprecation warnings, and now deal with it" we provide them with some additional helpful information which helps them to determine how critical a deprecation is:

warning: there was one deprecation warning (since 2.11.0)
warning: there were two deprecation warnings (since 2.12.0)
warning: there were five deprecation warnings (since FooBarLib 33.2)
warning: there were two deprecation warnings (since FooBarLib 32.8)
warning: there were three deprecation warnings (since BazProject 12)
warning: there were 13 deprecation warnings in total; re-run with -deprecation for details

A Scala developer already knows the Scala deprecation policy, therefore he/she has to check only FooBarLib's and BazProject's policies, instead of checking every single deprecation manually.

"has been" -> "is" was just to offset the potential width increase from the since information.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Apr 3, 2016

I like the uniformity for flavors of deprecannotations, and I liked the idea of emitting the since.

But when I saw the test output, as you say, it makes the lines longer. Maybe that's OK, I don't know; it probably just looks more repetitive in the test output. If you get just a few such messages, then you want all the info, and not "re-run with -Ydeprecation-explain".

@soc soc force-pushed the soc:topic/deprecations-since branch 2 times, most recently from ea66869 to 499a1a9 Apr 4, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Apr 10, 2016

Ok, maybe @lrytz has some time instead?

val warningVerb = if (numWarnings == 1) "was" else "were"
val warningCount = countElementsAsString(numWarnings, s"$what warning")
reporter.warning(NoPosition, s"there $warningVerb $warningCount in total; re-run with ${setting.name} for details")
}

This comment has been minimized.

@lrytz

lrytz Apr 29, 2016

Member

i would prefer to revert this part, i don't see a compelling value in including since in the summary.

This comment has been minimized.

@soc

soc May 1, 2016

Author Member

The idea is to give developers a better idea which deprecations will happen in the next version versus will happen in the future. It also makes deprecations more useful for non-standard-library authors because they can prefix the version string with their library name. So it's not "20 deprecations", but "9 deprecations from the standard library which will be removed in 2.13, and 11 deprecations from library X targeted for version 5.0".

This comment has been minimized.

@lrytz

This comment has been minimized.

@lrytz

lrytz May 2, 2016

Member

should we add a comment to the deprecated scaladoc, show some example output and give a recommendation to include the library name in the since parameter, and in what format?

This comment has been minimized.

@lrytz

lrytz May 19, 2016

Member

ping here - could you take a look at the scaladoc of class deprecated?

This comment has been minimized.

@som-snytt

som-snytt May 27, 2016

Contributor

Just tried it out, it's pretty neat. I don't know if I'd write since="funky 0.5". But as a future feature, the compiler knows where classes come from, and sbt knows about versions. The tool can tell me since 0.5 of funky, and you're using 1.0 already. That would be handy when you're trying out recipes and you don't even know what dependency an import comes from. Then you'd realize you're reading an outdated blog post.

A trivial improvement would be to sort by since in the summary?

res
}
def checkAltName = {
val res = checkDeprecation(false)
if (res) warn(s"the parameter name $name has been deprecated. Use ${param.name} instead.")
if (res) warn(s"the parameter name $name is deprecated$since: Use ${param.name} instead", version)

This comment has been minimized.

@lrytz

lrytz May 2, 2016

Member

make "Use" lower-case for consistency with other messages.

This comment has been minimized.

@soc

soc May 6, 2016

Author Member

I made it upper-case because the "standard" deprecation messages also used upper-case. Should I change both?

This comment has been minimized.

@soc

soc May 6, 2016

Author Member

Usages of "Use" in the standard library: https://gist.github.com/soc/2f348784e13a65f208585a09d6a94f48

This comment has been minimized.

This comment has been minimized.

@soc

soc May 19, 2016

Author Member

Ok, I'll change all usages to lower-case. Thanks!

reporter.warning(NoPosition, s"there $warningVerb $warningCount; re-run with ${setting.name} for details")
val warningsGroupedBySince = warnings.groupBy{case (pos, (msg, since)) => since}
val deprecationSummary = warningsGroupedBySince.size > 1
warningsGroupedBySince.map{warnings =>

This comment has been minimized.

@lrytz

lrytz May 2, 2016

Member

foreach instead of map?

This comment has been minimized.

@soc

soc May 8, 2016

Author Member

Will do! Thanks!

@soc

This comment has been minimized.

Copy link
Member Author

soc commented May 19, 2016

@lrytz Tests are running! :-)

By the way, what should we do with the policy-breaking "2.11.3" deprecations? Should we change them, so that the deprecation output becomes slightly shorter/more useful?

@soc soc force-pushed the soc:topic/deprecations-since branch from 499a1a9 to f981c6d May 19, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented May 19, 2016

@lrytz Updated!

@soc soc force-pushed the soc:topic/deprecations-since branch 6 times, most recently from 1c9a23c to ef26ca5 May 19, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented May 20, 2016

@lrytz I think I'm know happy with it. I revised @deprecated as well as @deprecatedName, @deprecatedInheritance and @deprecatedOverriding. All of them got code examples.

Could you review?

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented May 20, 2016

@soc i won't manage today, will look early next week..

@soc

This comment has been minimized.

Copy link
Member Author

soc commented May 20, 2016

Thanks!

* }}}
*
* The Scala language and its standard library will only add new deprecations on major releases to
* avoid the introduction of new warnings after minor version upgrades of Scala.

This comment has been minimized.

@lrytz

lrytz May 23, 2016

Member

cc @adriaanm - agree?

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

I'd like to reserve the possibility to introduce deprecations during minor releases. I think this is a good compromise to speed up library evolution while maintaining our 18 month major release cadence. Error reporting configuration could use the since parameter to make only deprecations introduced in .0 releases errors, while the later ones are always warnings.

This comment has been minimized.

@soc

soc May 25, 2016

Author Member

Mhh, ok. I only wanted to document the existing policy. (I tried to add minor deprecations in the past and was told, that I couldn't do that.)

Should I drop this bit?

This comment has been minimized.

@lrytz

lrytz May 25, 2016

Member

i'd say so.

This comment has been minimized.

@soc

soc May 25, 2016

Author Member

Done.

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented May 23, 2016

@adriaanm please also take a quick look at f981c6d#diff-bc1fa8bb16dc7dfb0992daa5a8051951 - do you agree this is a useful addition? it goes together with the new recommendation in the deprecated API doc here: ef26ca5#diff-6d4694cf4633f1feab80a318dbb398a1R16

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented May 23, 2016

I really like include the since info in the reporting! How about only doing the group-by when compiling under -deprecation? I agree we need to balance our error reporting budget (both columns and lines).

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented May 25, 2016

@adriaanm currently we print the summary ("there were n dep warns") only when -deprecation is not enabled. if it is, warnings are directly issues and you get the generic summary ("n warnings found").

do you propose to print the summary also under -deprecation, and in that case with grouping?

we could also not directly emit the warnings, but store them also under -deprecation. then we could report them by groups, instead of the order in which they come in.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented May 25, 2016

Good point. Maybe my suggestion isn't worth the additional complexity. I like the shaming of not having acted on depreciations introduced in 2.9 :)

@soc soc force-pushed the soc:topic/deprecations-since branch from ef26ca5 to d7f6d4d May 25, 2016

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented May 27, 2016

LGTM - unfortunately this needs another rebase..

@soc soc force-pushed the soc:topic/deprecations-since branch 2 times, most recently from 0fe9d60 to 372f6d3 May 27, 2016

@soc soc force-pushed the soc:topic/deprecations-since branch from 372f6d3 to 85057d5 May 29, 2016

@soc

This comment has been minimized.

Copy link
Member Author

soc commented May 29, 2016

Added @Ichoran's suggestion of ordering summarized warning messages. It's simply lexicographical order (so 2.10 comes before 2.9), but everything else would probably require tons of heuristics and guessing ...

@lrytz lrytz merged commit 8f567bc into scala:2.12.x May 30, 2016

6 checks passed

cla @soc signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [2163] SUCCESS. Took 16 s.
Details
validate-main [2439] SUCCESS. Took 139 min.
Details
validate-publish-core [2407] SUCCESS. Took 10 min.
Details
validate-test [2126] SUCCESS. Took 123 min.
Details

lrytz added a commit to lrytz/browse that referenced this pull request Jun 1, 2016

Fix signature of method overriding a method from the compiler.
Method `deprecationWarning` got a new parameter in
scala/scala#5076.
@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Jun 18, 2016

Sorting may have been my suggestion. @Ichoran knows the optimal way to sort.

Stopped by to say how neat the build looks.

[quick.library] warning: there were 38 deprecation warnings (since 2.10.0)
[quick.library] warning: there were 25 deprecation warnings (since 2.11.0)
[quick.library] warning: there were two deprecation warnings (since 2.11.6)
[quick.library] warning: there were 12 deprecation warnings (since 2.12)
[quick.library] warning: there were 21 deprecation warnings (since 2.12.0)
[quick.library] warning: there were 10 deprecation warnings (since 2.12.0-M2)

Maybe someone will add an ASCII graph with versions on the x-axis.

Or like the JIRA graph, comparing the pace of new deprecations to the rate at which we still incur them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment