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

Quality checks / Deprecated methods #7596

Merged
merged 4 commits into from Apr 3, 2019

Conversation

@bcardiff
Copy link
Member

commented Mar 26, 2019

Up to date description matching the final state of the merged PR.

This PR adds to build, run, and spec commands the --warnings all|none --error-on-warnings options. And an @[Deprecated("migration message")] annotation that can be applied to methods.

When compiling, warnings will be collected if a deprecated method is used. Depending on the CLI options the warnings will be reported or abort the compilation.

  • --warnings all will emit warnings but will keep generating the binary
  • --warnings none (default) will not emit warnings at all
  • --warnings all --error-on-warnings (or just --error-on-warnings) will emit warnings and will fail the compilation if warnings are found. Ideal for CIs.

In the spec command, when --error-on-warnings is used, the specs will run even if warnings are emitted. The exit status of the command will succeed only if both specs and warnings succeed.

NB: #7626 and #7661 complements this PR to reach the status described above


Original:

This PR adds to build and spec commands a --check option.

The --check option will notify if the emitted program is using any method marked as @[Deprecated("migration message")].

For the build command if --check detect issues the exit status is 1.

For the spec command I think it would be better to, even if quality checks errors are detected, to run the specs. But to do so Crystal::Command#execute needs to be refactored to overwrite the normal exit path. Before doing so, feedback is welcome.

I detect two issues, these are fixed in the PR but could be extracted if preferred.

  • The bash autocompletion was still referring to compile instead of the build command
  • The annotations were not cloned in Def

I didn't cover and I think it can be delayed:

  • Deprecation of a whole class/module.
  • Structured json output format of this messages
  • A proper refactor to product warning messages without creating a Crystal::TypeException

The tool as is, already help me detect lots of / that we need to change to //.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Related to #7027
In my opnion it would be better to have the deprecation warnings by default.
Another thing, why not using the stdlib logger? Then, having an argument to set the loglevel of the compiler.

@bcardiff bcardiff force-pushed the bcardiff:feature/quality-check branch from 86fd2c3 to 48a14c2 Mar 26, 2019

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@j8r I think that either

  • you care about future compatibility (ie: you want build to fail on the usage of deprecated message)
  • or not (ie: you don't even care to know about it).

Having a configurable error level for this check opens the door to compiler messages that will bother the output.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

For example in Rust there are Lint levels, I don't think this warnings are useless to not print them by default.
Else, I'm afraid only few aware people will benefit from it.

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

I'm supportive of adding support for proper deprecations, I think the annotation is good, but why are they opt-in? If I don't actively run with --check then I can totally miss a deprecation warning, and will later be faced with a broken compile.

Isn't check quite a broad term? It's not clear what it will actually check by its name alone. Unless there is a plan to check for different things later on?

I wonder about the failing status code, too. It will, eventually, fail to compile, but for now it did compile.

Maybe a --skip-deprecate would be more helpful and self explanatory?

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

Isn't check quite a broad term? It's not clear what it will actually check by its name alone. Unless there is a plan to check for different things later on?

No strict plan, but other checks I have in mind are deprecation of language constructs (probably with ... yield). And maybe some precision issues could appear. But what I don't like about detecting potential issues in a code base is that then we would need pragmas or something to allow them in specific situations. So I would stick initially regarding future compatibility of the code.

I wonder about the failing status code, too. It will, eventually, fail to compile, but for now it did compile.

There is an exit 1 in case warnigns were found, it should not compile.

@Sija

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

No strict plan, but other checks I have in mind are deprecation of language constructs (probably with ... yield). And maybe some precision issues could appear. But what I don't like about detecting potential issues in a code base is that then we would need pragmas or something to allow them in specific situations. So I would stick initially regarding future compatibility of the code.

Wouldn't extending (and perhaps integrate into the CI pipeline) Ameba be more beneficial?

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

@Sija ameba works at a syntax level so these checks can't be detected with it's current scope.

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

My thoughts on this (similar to those of @ysbaddaden):

  • Why it's a --check flag and not a check (or a better name) command? I can't see why I'd like to build a program or run specs and also check that I don't use deprecated things. I'd only want to do one of those things.
  • check sounds too broad.
  • This is an optional thing to do in your project, so "if you want to use it, you use it, otherwise you don't" initially sounds fine... until we remember that Crystal will compile all of the code at once, including your code and that of libraries. If a library doesn't care about deprecations they will pop up on your checks and there's nothing you can do about it. Well, you can bug the library's author to fix those deprecations. But I think a much better way to accomplish this is to always show these deprecation warnings. Because you can't remove these warnings, you'll be almost forced to fix them right away. That way libraries will stop using deprecated features as soon as possible. And note that it's just a warning: the compile output will be polluted, but your code will still compile and run fine. But because it's annoying you will most likely get rid of that (and then you could have --check or --treat-warnings-as-errors flag).
@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

What about having --warnings=emit|error|ignore and --exclude-warnings=paths (in build and spec) to allow the following use cases:

  • (default) --warnings=emit --exclude-warnings=lib. Bother the author with respect it's app/shard code but let it compile them.
  • --warnings=error good for CI since it will change the exit status
  • --warnings=ignore turn warnings off
@konovod

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

There could be times when you just can't fix some warning (the feature doesn't work correctly in current compiler release, or you have to support previous release, or you are using old unsupported shard and don't have a knowledge\time to fix it). These situations are, of course, temporary, so it is ok to have warnings on by default, but it would be nice to have an ability to disable some on them - either using a compiler switch or by annotating in code.

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@bcardiff your proposal sounds good.

Note: I like how Rust as an optional since parameter (Rust RFC 1270), that improves the message and gives a reference version to look at for the deprecation:

#[deprecated(since="1.2.3" note="use bar() instead")]
fn foo() {}

// => warning: foo() is marked deprecated as of version 1.2.3
//    note: use bar() instead
@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

@ysbaddaden But the since should be the version each package I guess that that information is not available in a uniform way. For now the deprecated annotation can accept only the message and we can extend that later. The since: can be simulated by

  {% if compare_versions(Crystal::VERSION, "0.29.0-0") >= 0 %}
    @[Deprecated("Lorem ipsum")]
  {% end %}

I guess the since: only make sense if we want to deprecate this in a future version that is not the very next one. Otherwise, the deprecation could just be commited. Right?

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Unless that since is used programmatically somewhere, I think just doing @[Deprecated("Deprecated since .... Please use ...")] is the same.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Giving a time frame of the deprecation should always help. But I suppose it would even be better to announce the future version in which the feature is scheduled to be removed.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@asterite Even if not used programmatically, allowing separate keys helps to provide standardized information in a uniform manner and can be used for example in API docs. Or for compiling a list of deprecations by version.

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

The following pattern can be used to schedule a deprecation and removal.

{% if compare_versions(Crystal::VERSION, "0.29.0-0") <= 0 %}
  {% if compare_versions(Crystal::VERSION, "0.28.0-0") >= 0 %}
    @[Deprecated("Lorem ipsum")]
  {% end %}
  def foo
    # ...
  end
{% end %}

We can add additional information but I would prefer to find a way to enforce that semantic if the information is available. But I think we can start without them.

Without a way to know which is the package version we will need to inform it to the annotation.

@[Deprecated("Lorem ipsum")]
def foo
end

@[Deprecated(note: "Lorem ipsum")]
def foo
end

@[Deprecated(note: "Lorem ipsum", version: Crystal::VERSION, since: "0.28.0")]
def foo
end

@[Deprecated(note: "Lorem ipsum", version: Crystal::VERSION, since: "0.28.0", drop: "0.29.0")]
def foo
end

Again, let's start with the note and the cli options and leave this ideas for the future.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

But I think we can start without them.

Definitely. We can ship a MVP without that and collect experience on how it can be used and what features we're missing.

@bcardiff bcardiff added this to the 0.28.0 milestone Mar 27, 2019

@bcardiff bcardiff force-pushed the bcardiff:feature/quality-check branch 2 times, most recently from abb5744 to 3d17268 Mar 28, 2019

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

The last push implements #7596 (comment) for the run, build and spec commands.

It's ready for another round of reviews.

@bcardiff bcardiff requested review from straight-shoota and asterite and removed request for asterite Mar 28, 2019

@asterite
Copy link
Member

left a comment

Looks good. I left some comments.

Show resolved Hide resolved src/compiler/crystal/codegen/warnings.cr Outdated
Show resolved Hide resolved src/compiler/crystal/command.cr
Show resolved Hide resolved src/compiler/crystal/command/format.cr Outdated

@bcardiff bcardiff force-pushed the bcardiff:feature/quality-check branch from 5ca065a to 13bc863 Mar 29, 2019

Show resolved Hide resolved src/compiler/crystal/semantic/ast.cr Outdated
Show resolved Hide resolved src/compiler/crystal/semantic/ast.cr Outdated
Show resolved Hide resolved src/compiler/crystal/semantic/ast.cr Outdated
Show resolved Hide resolved src/compiler/crystal/semantic/top_level_visitor.cr Outdated
Show resolved Hide resolved src/compiler/crystal/semantic/top_level_visitor.cr Outdated
Show resolved Hide resolved src/compiler/crystal/codegen/warnings.cr Outdated
Show resolved Hide resolved src/compiler/crystal/program.cr Outdated
Show resolved Hide resolved src/compiler/crystal/semantic/top_level_visitor.cr Outdated

@bcardiff bcardiff force-pushed the bcardiff:feature/quality-check branch from 13bc863 to 452aa38 Mar 29, 2019

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

Last comment and naming feedback incorporated. Thanks @j8r.
Rebased & squashed on master.
Ready to merge after CI is happy :shipit: .

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

Oh, I will give some time and see if @straight-shoota or @ysbaddaden have more feedback before merging.

Show resolved Hide resolved src/compiler/crystal/command.cr Outdated

@bcardiff bcardiff force-pushed the bcardiff:feature/quality-check branch from 452aa38 to bdccdcb Mar 29, 2019

compiler.warnings_exclude << Crystal.normalize_path "lib"
opts.on("--warnings emit|error|ignore", "How to treat warnings. (default: emit)") do |w|
compiler.warnings = case w
when "emit"

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Apr 1, 2019

Member

In this context, emit might be understood to refer to emitting code. Why not call it warn as a less strict version than error?

This comment has been minimized.

Copy link
@asterite

asterite Apr 1, 2019

Member

We should probably not try to invent the wheel. I think everywhere else in the world there are two options: "warnings on or off", and "treat warnings as errors".

This comment has been minimized.

Copy link
@ysbaddaden

ysbaddaden Apr 1, 2019

Member

You're maybe right. I usually associate emit as the output format (object file, LLVM IR, JSON...).

This comment has been minimized.

Copy link
@bcardiff

bcardiff Apr 1, 2019

Author Member

Ok, do we prefer to start with --warnings all|none and --error-on-wargnings in case the
warnings categories increase in the future so we can have --warnings deprecated,X,Y,Z to specify which warnings we want?

Or seems enough to go with --warnings warn|error|ignore?

This comment has been minimized.

Copy link
@j8r

j8r Apr 1, 2019

Contributor

Having warning categories is great. For instance on pylint, there is a -d, --disable= to disable messages/reports. I think this will be appropriate to copy this opt-in by default approach, for example having disable-warnings , and only disable warnings we don't want.

There is also a disable=all to disable all messages/reports, and enable to enable some.
For example disable=all enable=classes

This comment has been minimized.

Copy link
@asterite

asterite Apr 1, 2019

Member

I think --warnings all|none with --error-on-warnings is fine. Then we could eventually list specific warnings, as you suggest.

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

The last push implements the --warnings all|none --error-on-warnings CLI options.
Although Crystal::Warnings could be a @[Flags] there is currently no need.

Once reviewed I will rebase + squash for merging.

cc: @straight-shoota @asterite

@j8r

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

It would be also good to have a way to send warnings without relying only on annotations. It's not always possible to use them, for example when changing a syntax in the compiler.
One can say we have already {{ puts "deprecated notice" }}, but that's not really a clean API for deprecation warnings.

bcardiff added some commits Mar 25, 2019

Fix bash autocompletion
It seems that bebdd56 didn't revert fully #2737
Add --warnings --error-on-warnings --exclude-warnings compiler options
* Add warnings checks for deprecated methods
* Support warnings for build, run and spec commands
* Extract path handling operations of formatter to reuse them
* Defaults set to --warnings=all --exclude-warnings=libs

@bcardiff bcardiff force-pushed the bcardiff:feature/quality-check branch from 4c10a38 to b019662 Apr 3, 2019

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@j8r feel free to open an issue to discuss the design around that. Probably a deprecated macro method would work to check other constructs like deprecating specific C library version.

The compiler itself has everything needed to add ad-hoc code to detect usage of language constructs to be deprecated in the future.

@bcardiff bcardiff merged commit 40d8b34 into crystal-lang:master Apr 3, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff deleted the bcardiff:feature/quality-check branch Apr 4, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Do we want to include --check in the compiler build and spec runs on CI?

@bcardiff

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

In the compiler specs we can. In the std lib it could only be when running against the compiled compiler.

Once 0.28 is released, definitely!

bcardiff added a commit to bcardiff/crystal that referenced this pull request Apr 8, 2019

@bcardiff bcardiff referenced this pull request Apr 8, 2019

Closed

Remove compiler warnings #7652

bcardiff added a commit to bcardiff/crystal that referenced this pull request Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.