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

Annotation for suppressing specific compiler errors #11044

Open
HertzDevil opened this issue Jul 31, 2021 · 5 comments
Open

Annotation for suppressing specific compiler errors #11044

HertzDevil opened this issue Jul 31, 2021 · 5 comments

Comments

@HertzDevil
Copy link
Contributor

I propose a new annotation that turns selected compiler errors into warnings, and turns warnings into nothing.

This annotation is designed for smooth migrations in the middle of a major Crystal release where the migrations are initiated from Crystal code (and not from e.g. a new language feature). For example, consider the task of adding a return type to an abstract def in the standard library (e.g. #10903). Below is how the annotation would look like:

module Iterator(T)
  @[Suppress(:abstract_def_return_type)]
  abstract def next : T | Iterator::Stop
end

class Foo
  include Iterator(Int32)

  # signature matches the abstract def except for the return type
  def next; 1; end
end

This would generate a warning instead of error:

Warning: this method overrides Iterator(T)#next() which has an explicit return type of (Iterator::Stop | T).

Please add an explicit return type ((Iterator::Stop | T) or a subtype of it) to this method as well.

The above warning will become an error in a future Crystal version.

The idea is that the Suppress annotation will remain throughout the rest of 1.x, and then it will be manually removed for the 2.0 release, in a similar manner as @[Deprecated]. This gives end users a rather large time window for implementing those migrations, instead of doing nothing until the end of 1.x and then suddenly seeing the new return type restrictions just in time for 2.0.

Suppress accepts a single Symbol argument to indicate the compiler error / warning type. To support the addition of completely new abstract defs, we simply use a different argument:

abstract class Foo
  @[Suppress(:abstract_def_impl)]
  abstract def foo
end

class Bar < Foo; end

# Warning: abstract `def Foo#foo()` must be implemented by Bar
#
# The above warning will become an error in a future Crystal version.

A use case for suppressing warnings is renaming a type Foo to Bar:

@[Deprecated]
class Foo; end

@[Suppress(:deprecated_name)]
alias Bar = Foo

Assume that #11043 is implemented, so that any use of Foo results in a deprecation warning. But we don't want all of this; uses of Foo through Bar should not be deprecated, as is the reference to Foo in Bar's definition itself. This annotation will hide those warnings. (We cannot make the old type an alias of the new type since that breaks existing code that depends on Foo.name.)

@straight-shoota
Copy link
Member

I think it's worth mentioning that the described approach to let missing return types of abstract methods be a warning for the remainder of 1.x which becomes an error in 2.0 could just as well be done without annotations.

The benefit of using specific annotations is that we can selectively choose between a migration path for pre-existing features (keeping the old, non-restrictive semantics plus warning), and have the new, restrictive semantics apply for newly added features.

I'm unhappy however about the proposed opt-out approach. It would require all existing code to be changed in order to suppress warnings. Without @[Suppress], enabling the compiler feature would lead to breaking changes. I don't think this can be done while keeping backwards compatibility.

Still, we can go the other way around and add an annotation to opt-in for more restrictive compiler behaviour. This annotation can be applied to newly added features and makes sure the compiler enforces strict semantics from day one.

Suppressing something that should've been error feels quirky. But opting in for more errors is actually very healthy.

@HertzDevil
Copy link
Contributor Author

I think it's worth mentioning that the described approach to let missing return types of abstract methods be a warning for the remainder of 1.x which becomes an error in 2.0 could just as well be done without annotations.

How would you do this without inadvertently relaxing the same requirement for abstract defs that are not part of a 1.x changeset?

@straight-shoota
Copy link
Member

Yes, that's not possible and we need annotations for that as I wrote in the following paragraph.

@HertzDevil
Copy link
Contributor Author

So what you're suggesting is instead of adding a "non-strict" annotation for new methods, we revert the error behaviour but add a "strict" annotation to every existing abstract def in the standard library and every shard out there?

@straight-shoota
Copy link
Member

No that's not at all what I meant. I suppose it depends on the type of change.

After failing to communicate my intention to you, I realized that I had missed the precondition that you're talking about errors being introduced by change in Crystal code, not changing compiler behaviour. My comment was about changing compiler behaviour, so it does not apply to cases like #10903.

Sorry for the confusion.

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

No branches or pull requests

3 participants