[RFC] Deprecation tool

[bcardiff]

Ref: #7652

The current idea to handle deprecation is to create a separate tool that will check if deprecated definitions are used. This way the compiler can stay warning free and the tool can be used before migrating/updating std/dependencies on demand.

Input from the community is welcome. We want to have a tool that will help shards authors and app developers to provide/have a smooth migration path for breaking changes.

CLI vs integrated with the compiler

Making it a separate tool has on the pro side that:

• It keeps the compiler code agnostic to this analysis.
• Helps to handle better support for editors.
• The compiler stays warning free (no poluted output).

and on the cons side:

• The user might not be pushed to migrate code until it's too late (ie: the deprecation was removed).
• It is less clear for the user how to run the tool against specs. crystal spec directly grabs ./spec/**_spec.cr.

Output

There were some concerns regarding how verbose the output should be (by default). The options discussed where:

• Show if a deprecated definition is used. Showing only one notice per definition (not one per invocation).
• Show the location of each invocation of a deprecated definition plus the notice.
• Show the snippet of code as the context of the deprecated invocation notice.

The current initial proposal is to:

1. Create crystal tool deprecation path/to/main.cr [compiler build options]
2. Output the location of each invocation of a deprecated definition plus the notice.
3. Have the exit status be non zero if there were deprecations detected.
4. No tool option other than the compiler build options.

# file main.cr
@[Deprecated("Use `#bar` instead2")]
def foo
end

foo
if true
  foo
end

$ crystal tool deprecation main.cr
main.cr:6:1 Deprecated top-level foo. Use `#bar` instead2
main.cr:8:3 Deprecated top-level foo. Use `#bar` instead2
$ echo $?
1

If no deprecations are found:

$ crystal tool deprecation main.cr
There seem to be no usages of deprecated definitions.
$ echo $?
0

[refi64]

It seems a bit odd to have a tool just for deprecations... What would be the chances of naming it e.g. "lint" and adding anything else that might be seen as a warning to it later on?

[j8r]

Agree with @refi64. The question is: what are the reasons one wouldn't want warnings?

• I don't want to be informed of deprecated methods of my project; this isn't very responsible, but a flag can be set to ignore them.
• I don't want to be informed of my libraries deprecated methods; that may be legit, I think the general linter tool (not one specific to deprecations) can be configured to ignore/output warnings for each lib through the build CLI.

TL;DR: at least enabling the wanings for the main project (./src) seems to be the best, and a responsible good practise. They can be disabled by default for the libraries (./lib), we can't always do something about them and may be considered as spam.

[straight-shoota]

@refi64 We actually try to avoid building a linter into the compiler.

The compiler is focused on compiling code (and either that works or it fails, no warnings). But it provides a few essential features around that which are considered important enough to have a standard implementation in the compiler. compiler tool format applies a standardized formatting. This is of course opinionated, but it's well worth having it to avoid useless discussions about how to format code.
crystal tool deprecation would essentially add support for stdlib's Deprecated annotation to the compiler, as a tool for developers to analyze the usage of deprecated features in their code base. Providing help for migrating code is important and it should be easy enough to implement. And most people should be able to agree on how it behaves.

A linter on the other hand is complex and very opinionated. It's probably impossible to get a broad consensus on its rule set. This should be provided by an external tool.

[bcardiff]

I forgot to include that a CLI option --exclude path to exclude reports from those paths having as default ./lib should be included.

[bcardiff]

From the PR that the warnings were included, I am more in favor of having this built-in directly in the compiler. But some people prefer warnings free compiler.

I care about having some support for smooth migration as early and as easy as possible.

[asterite]

Here's my opinion: having this as a separate tool is simply hiding it. Who would care about running this tool to see deprecation warnings and then trying to fix them? Or who would remember to do it? And how, a periodical check? It's just not a convenient flow in my opinion.

Have it built-in into the compiler and turned on by default: you can immediately see what you can change in your code right now to avoid some pain later when a new release is out. You can't ignore this: it's turned on by default. You can ignore it once you saw it once, but at least you saw it! Then maybe the fix list isn't that long and one can fix those deprecation warnings and move on. If the list is long one can choose to still see it so one doesn't forget about it (it doesn't bother compilation at all, it's just text that's output at the end of compilation).

Also, this is yet another tool that comes with the compiler and must be learned. And I was personally more inclined to move these tools outside of the main compiler.

Also, linting and deprecation warnings are different things. Deprecation doesn't have to do with code style or recommendation: deprecated method will be removed in the future.

[j8r]

It may be the opportunity to have a survey, no? A bit like Go and Rust. There was one in 2017, but not in 2018 as I remember. Other questions can also be asked (about language usage, expectations, funding...)

[bcardiff]

If it is to be a tool I would call it something more generic than deprecation. Maybe crystal tool doctor, where the limit of the responsibility is to identify some quality aspects of the code and not stylistics. By quality aspects I mean identify things that although work/compile, it would be better if done differently due to maintainability or performance issues.

But the deprecation cycle is too important and so much used that I still prefer it bundled it directly in the compiler. Even if it's opt-in for a couple of releases until more feedback is gathered.

[ysbaddaden]

I fully agree with Asterite. A deprecation is a friendly warning that your code will break. I don't understand the idea of running an "additional" tool to get deprecations, which feels like burying these messages :/

[RX14]

@asterite I think it was you who first made me realize how compiler warnings affect the UX of the compiler CLI. The crystal tool's experience has always been one of cleanliness, utility and minimalism. When you deprecate a method such as Time.now, or Int#/ the number of call-sites is in the thousands. Providing the current extremely verbose (5 lines of text per invocation!) output by default will produce many many pages of warning output: scaring newcomers; and depressing veteran crystal programmers with large codebases with the scale of the refactor.

But I think an important part of my proposal was missed: in fact I suggested deprecation warnings will always be on in crystal build, with no way to ignore them. But they will be printed once per deprecation instead of per invocation. The reality is that you don't need to know about every usage of a deprecated method every compile, in fact almost all the time you will perform the refactor to use the new method all at once. The only immediate information you need to know is that you are using a deprecated method so that you are reminded to fix it.

So the workflow becomes this:

# update stdlib/library version

$ crystal build src/my_project.cr
Deprecated method `Time.new` was used! Use `crystal tool deprecations` to identify usages and replace with `Time.local`.
Deprecated method `Int#/` was used! Use `crystal tool deprecations` to identify usages and replace with `Int#//`.

# Compiled output is produced normally, with a short message *every time you compile*
# reminding you that you use deprecated methods. Once you are ready to migrate (which in a
# company with priorities and schedules might *not* be now) you run crystal tool deprecations.

# Keep working normally:
$ git commit

# It's now time to fix the deprecations
$ crystal tool deprecations
src/my_project/foo.cr:6:1 Deprecated usage of `Time.now`. Use `Time.local` instead
[ 3 times ]
src/my_project/foo.cr:8:3 Deprecated usage of `Int#/`. Use `Int#//` instead
[ 10 times ]
$ vim src/my_project/foo.cr
$ git commit -am "Update to crystal 0.28.0"

I would even suggest replacing crystal tool deprecations with crystal tool usages Int#/ which is a much more powerful and generic tool to build.

If you have just a single invocation of a deprecated method in your project, this way of presenting deprecations produces marginally more work, but for projects with many deprecations, the difference in user experience between the two approaches is stark: possibly 5 pages of text vs 2 lines. You request the 5 pages of text when you need that verbosity, instead of all the time.

So what's the difference between this and opt-in compiler warnings? Compiler flags and configuration complexity has been replaced by convention and an additional tool (which could easily be renamed and made more generic to IDEs and editor plugins).

[asterite]

possibly 5 pages of text vs 2 lines

Did we already see something like that? Or it's just a possible fear? Because 5 pages of text might not be a reality at all.

[RX14]

@asterite Using a compiler built from the current master with #7586 applied and Int#/ deprecated, I used amber new foo to generate a new amber project. I compiled it and took the output. It's far too long to paste in this github issue so I put it in a gist here: https://gist.github.com/RX14/db4938089f882b91d55120b946540aee

It's 2319 lines or 124kB of text! That's just for 67 warnings in a completely empty app!! I find that unacceptable.

[bcardiff]

Deprecating #/ flagged things in order of a few hundred and from an iterative process of checking std specs and compiler I reach #7639. @RX14 the output you sent is not including the updated stdlib with those replacements. The warnings should be about the app and some leftovers of the stdlib.

@RX14 then I don't understand why you were against shipping the warning check in 0.28 behind an opt-in, since your proposal includes a less verbose output, but it does include an output.

crystal tool usages Int#/ is already implemented, but it still needs more work. The api needs to point to a cursor location to be able to express exactly which override is wanted. Not just method name.

Within the feedback in this issue, plus your example above and comments in #7652 the best things is to deliver the warning check with opt-in. The output can evolve in the next versions and we can have at least some way to support deprecations.

[RX14]

@bcardiff yeah I realise after reading the output that it's just the standard library. It also shows a bug with the entire macro expansion showing as context for one deprecated method. I'd rather not show any context though.

I think that even ignoring those two mistakes, there's a lot of messy output for a large project with a lot of usages of such methods.

@RX14 then I don't understand why you were against shipping the warning check in 0.28 behind an opt-in, since your proposal includes a less verbose output, but it does include an output.

Largely because people will start using the --warnings flag, then get fustrated when it's removed it in the next version. There's no point adding a compiler flag which people will use in CI when it's going to be removed next version and break their CI. I'm not super opposed to leaving it opt-out but I don't like it.

crystal tool usages Int#/ is already implemented, but it still needs more work. The api needs to point to a cursor location to be able to express exactly which override is wanted. Not just method name.

Agreed.

[asterite]

Unrelated, bot not that much: for macro expansion code we should probably just show the expanded line that failed, not the entire macro. That could improve the experience a bit.

[bcardiff]

Largely because people will start using the --warnings flag, then get fustrated when it's removed it in the next version.

But now nobody is saying that it should be removed, the discussion will be about what should be the compilers output when reaching a deprecation method.

People that will use --warnings will be participating to have something they found useful.

There's no point adding a compiler flag which people will use in CI when it's going to be removed next version and break their CI. I'm not super opposed to leaving it opt-out but I don't like it.

Let's make it opt-out opt-in and will be clear that the output is subject to change and we are expecting feedback.

[bcardiff]

#7661 created to make it opt-in

[RX14]

But now nobody is saying that it should be removed, the discussion will be about what should be the compilers output when reaching a deprecation method.

I proposed that it be removed above:

in fact I suggested deprecation warnings will always be on in crystal build, with no way to ignore them

Either way, as long as the default behavior doesn't change in this release I'll begrudgingly approve it.

[ysbaddaden]

Printing the same message many times is verbose, but it shows how many times an issue is present, and where they all are. Running again shows the remaining places to fix. Verbosity can be scary but it's explicit, and less output in subsequent builds is rewarding.

Printing a message once shows one issue location, fix it, build again, but fail again with another location, and repeat with no end in sight. It's less scary but can be very frustrating.

[straight-shoota]

The compiler's responsibility is to compile a program. Obviously, if it fails to do that, it needs to print an error. But deprecated features are not an error. Now the compiler might add some information to inform about the usage of deprecated features. That can be seen as a convenience feature to encourage developers to migrate.

But in it's essence, using a deprecated feature is perfectly fine. It's not recommended to do so, but the code compiles and doesn't really pose a situation which requires to focus immediate attention, disregarding all other work. That's not very realistic anyway.
Depending on the deprecation schedule, the code might keep working for a number of releases to come (that's why I'd like @[Deprecated] to have an attribute for estimated removal).

Replacing the use of deprecated features is not always as simple as s/Time.now/Time.local/. When more complex components are deprecated, it can be quite some effort and require larger refactoring to migrate away to a supported alternative. And these might not be possible to implement right away because they're blocked by other features or general limitations like time and money. We know that software engineering processes can be more difficult than it would allow to have all deprecations immediately fixed. In such situations, I wouldn't want the compiler to constantly scream about dozens or hundreds of uses of deprecated methods when I'm well aware of them but they can't be fixed right away. It also happens, that new warnings which I might not yet be aware of, simply vanish in a long stream of messages (disabling warnings entirely has the same result).

Having a simple list of deprecated features that are used in the code is less obstrusive and encourages users to actually follow it. Changes like a new deprecation coming in from updating some dependency are easy to spot. Once all uses of a deprecated feature have been removed, it will disappear from this list.

[straight-shoota]

#7639 is a good example why deprecation warnings do not need to be treated as super-important fix-me-or die messages, constantly clogging the compiler output. The deprecation message for Int#/ announces that its return type will change from Int to Float. If your code expects that or don't care (because the result is always an integer and Number is all you need - or whatever reason), this deprecation message is completely safe to ignore. For this is specific example, this would probably be rare. But I'd like to think of using deprecation warnings not only for removal of features (which would eventually be noticed anyway) but also to more subtle changes in the API, which are otherwise not really noticeable and might only affect some use cases. For example, URI#path has changed in 0.28.0 that it no longer returns Nil but also the value of an opaque path. If you expected an opaque path to return Nil, this would be a relevant API change which would be announced in a deprecation warning (see #6323 for details).

[j8r]

Except that the Int division isn't deprecated, but behave differently. It should be on a lower priority logging than warning, like information/note.

[straight-shoota]

It should be on a lower priority logging than warning, like information/note.

Why? It's a deprecation of an API feature. Changing the semantics of a method is similar to removing it. Both may lead to different behaviour and failing builds in the next release.

[didactic-drunk]

I don't supposed the tool will have sort of parsable (json?) output so I can figure out how to feed every deprecated warning to my editor and fix them all at once?

[bcardiff]

@didactic-drunk no, it could be added, but after some iteration on the tool, I would say.
The relevant code is in https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/codegen/warnings.cr#L93-L100 . And https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/codegen/warnings.cr#L69