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

Runtime API deprecation warnings #761

Merged
merged 10 commits into from
Jan 23, 2018
Merged

Runtime API deprecation warnings #761

merged 10 commits into from
Jan 23, 2018

Conversation

ducky64
Copy link
Contributor

@ducky64 ducky64 commented Jan 20, 2018

Add runtime warnings for use of deprecated Chisel methods. This is done using a macro that takes the message from a @deprecated annotation, and adds a call to Builder.deprecated.

Reasoning is that by default, Scala doesn't print all deprecations, and that it's somewhat tricky to notice them - yet some support questions revolve around the use of deprecated and terribad API. This now prints warnings for uses of deprecated functions at runtime, and aggregates them by error and line to avoid spam. Also included is convenient information on enabling scalac deprecations.

This also changes how line numbers for Chisel's error facility is determined, using prefix string comparison of the stack trace element classnames, instead of checking if the class is a subtype of UserModule. The previous one (specifically, calls to Class.forName) seems to interact badly with reflection-based cloneType when called at scale. This should also give more accurate reporting of errors that are in user code but outside of a UserModule.

It turns out that @deprecated on macro functions don't do anything, so this changes the tags to the functions that the macros point to, which seems to work properly. It also turns out that there's a bunch of uses of deprecated functions in chiselTests which needs to be fixed.

Not all @deprecated functions are also annotated with @chiselRuntimeDeprecation, because they're still used in Chisel internals, and we can't track whether they're called by the user or by Chisel and it will give a misleading error. These are a small amount of functions.

  • Related issue (if applicable)

  • Type of change

    • Bug report
    • Feature request
    • Other enhancement
  • Impact

    • no functional change
    • API addition (no impact on existing code)
    • API modification
  • Development Phase

    • proposal
    • implementation
  • Release Notes

Add runtime deprecation warnings and improve error reporting.

@ducky64 ducky64 requested review from ucbjrl and jackkoenig January 20, 2018 01:13
@ducky64 ducky64 added this to the 3.1.0 milestone Jan 20, 2018
@ducky64
Copy link
Contributor Author

ducky64 commented Jan 20, 2018

Updated to remove most deprecation warnings during runtime.

Only deprecation message that pops up now during a rocket run is:

[deprecated] Debug.scala:928 (1 calls): $bang$eq is deprecated: "Use \'=/=\', which avoids potential precedence problems"

Side note: the nasty code introduced in ReadyValidIO and Valid needs to be removed eventually. Because those are in chisel3._, they are under new semantics even if invoked from compatibility code, which is why the Builder.currentModule detection is needed. Ideally rocket would fix this at the source.

@ducky64 ducky64 mentioned this pull request Jan 20, 2018
8 tasks
@chick
Copy link
Contributor

chick commented Jan 22, 2018

This looks pretty good to me. A couple of questions:

  • Is there any kind of test that would show this in action and/or make sure code is run?
  • Is there any corresponding wiki page showing example output?
  • Does this represent a methodology for deprecating things in the future and if so can we make a wiki page for that?

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 22, 2018

This is solely a best-effort deprecation scheme that tries to make deprecations a bit more noisy than what they are in scalac by default, and also tell them how to turn on detailed deprecation warnings in scalac. It also makes it clear to the user that deprecated functions may go away and that they should fix their code.

Tests: this is hard to do, because it's all printlns. It also shouldn't affect the output at all, so I'm less concerned about testing.
Wiki page: this is meant to be self-explanatory, basically taking the scalac @deprecated annotation and also making sure messages are reported during runtime
General deprecations: Use @deprecated(message, since) and stack this runtime deprecation annotation on top of it to also make it complain during runtime (if that's what you want).

Copy link
Contributor

@ucbjrl ucbjrl left a comment

Choose a reason for hiding this comment

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

lgtm

@jackkoenig
Copy link
Contributor

Did this change deprecate != in compatibility code? It's not that big of a deal, most compatibility code uses =/= anyway

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 23, 2018

Yeah, !=, even in compatibility mode, ends up triggering runtime deprecations because it was already triggering compile time deprecations.

@jackkoenig
Copy link
Contributor

What I mean to say is that I have code that didn't get the compile-time deprecation warnings before, and now does. I'm not sure why that is but it was only 3 places so 🤷‍♀️

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 23, 2018

Ah. Yeah, I think this was one of those cases where the deprecated annotation was on the macro (for SourceInfoTransform) instead of the macro target, so it didn't fire before. But this PR also corrects that, so it fires now.

@jackkoenig
Copy link
Contributor

Anyway, this lgtm!

@ducky64 ducky64 merged commit 224740a into master Jan 23, 2018
@ducky64 ducky64 deleted the apideps branch January 23, 2018 21:54
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.

4 participants