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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler: correctly check abstract methods #7956

Merged
merged 6 commits into from Jul 24, 2019

Conversation

@asterite
Copy link
Member

commented Jul 9, 2019

Fixes #3546
Fixes #6085
Fixes #6762

This PR does a few things.

Return types are checked 馃帀

Return types of abstract methods are now checked in subtypes.

It took me a long time to implement this because I didn't know how to resolve this issue: the abstract method check runs after the first compiler pass where all types and methods are defined, but before method bodies are typed (and so, actual method return types are computed). So by the time we could check whether an abstract method was correctly implemented (the return type matches) a different error might have been triggered.

Also, if you have something like this:

abstract class Foo
  abstract def foo : String
end

class Bar < Foo
  def foo
    1
  end
end

what would the error be? It will be something like "Method Bar#foo must return String because it implements the abstract method Foo#foo which returns String", which is a mouthful of words. But also looking at the code it's not clear that Bar#foo must return a String, because that code and the abstract method are far away (probably in a different file).

One way to solve this, which is what this PR does, is to force subtypes to include an explicit return type if the abstract method has one. Once this is required we can check whether the return type matches without actually typing the body. Actually checking whether the body complies to the explicit return type always comes next, at some point.

This is backwards incompatible because for existing code you will be required to add explicit return type annotations. However, I think this is good: it solves the problem I mention above where it wasn't clear that Bar#foo was supposed to return String, because now that will be explicitly written. (well, the connection with the abstract method is still missing, and for that maybe I'd like to make an Overrides annotation mandatory, at least for this case... but not a part of this PR or discussion).

So for instance, in the above code you will now get this error (featuring the super nice error formatting made by @martimatix:

In foo.cr:6:7

 6 | def foo
         ^--
Error: this method overrides Foo#foo() which has an explicit return type of String.
Please add an explicit return type (String or a subtype of it) to this method as well

Then let's say you go ahead and modify it to this:

class Bar < Foo
  def foo : Int32
    1
  end
end

You get this error:

In foo.cr:6:13

 6 | def foo : Int32
               ^----
Error: this method must return String, which is the return type of the overwritten
method Foo#foo(), or a subtype of it, not Int32

Changing Int32 to String and fixing the body compiles as expected.

(the error messages are still long, but I think the resulting code is clearer and more explicit)

Also note that the error message says it must return the base type or a subtype of it. For example, this is valid:

class Parent; end
class Child < Parent; end

abstract class Foo
  abstract def foo : Parent
end

class Bar < Foo
  def foo : Child
    Child.new
  end
end

The code above is fine. After all, Child is a Parent so if you didn't know whether you have a Foo or a Bar you still can treat it as a Parent (and this is how other statically typed languages with subtyping work).

Finally, the third commit in this PR adds some explicit return type annotations to comply with this change. I was able to fix it in a few minutes, so even though this is a breaking change it should be forward to fix it. And if not, I also added a flag, skip_abstract_def_check, which will skip this check altogether. This is useful in case you can't upgrade right now, or in case I got something wrong in the check algorithm.

I also fixed a couple of incorrect things:

  • for Random#next_u there was no UInt type (and some Random instances return UInt32, some UInt64, some UInt8... maybe this should be unified, I don't know... but not part of this PR or issue)
  • for Socket::Server (or something like that) the return type was incorrect
  • Deque included Comparable but never implemented <=>: I decided to remove the inclusion of Comparable because I think we can move that to Indexable, but I'd like to do it in a different commit (and nothing is lost here anyway because it could never be used as a Comparable).

Abstract methods are now correctly checked for generic types! 馃帀

So for example this works fine:

abstract class Foo(T)
  abstract foo(x : T)
end

class Bar(U) < Foo(U)
  def foo(x : U) # OK
  end
end

class Baz < Foo(Int32)
  def foo(x : Int32) # OK
  end
end

The compiler knows that the T in Foo in Baz is an Int32 so it checks against that. Kind of obvious, I know, but it wasn't that straight-forward to implement. And for return types it will suggest the correct (instantiated) type.

And this also works for generic modules. So previously these were all skipped (because the implementation wasn't correct, so it was turned off... though for generic classes it was turned on but it worked incorrectly! see #6762). So this code used to compile:

class Foo
  include Enumerable(Int32)
end

But now it correctly gives an error:

In src/enumerable.cr:37:16

 37 | abstract def each(&block : T -> _)
                   ^---
Error: abstract `def Enumerable(T)#each(&block)` must be implemented by Foo
@petr-fischer

This comment has been minimized.

Copy link

commented Jul 10, 2019

I think this could be enough (the return type of the implemented abstract method might not be mandatory, just deduce the type from method body and check it against abstract def return type, if it's given - that would be the perfect Crystal way):

abstract class Foo
  abstract def foo : String
end

class Bar < Foo
  def foo
    "aaa"
  end
end

but I understand, that it's hard to implement - by your comments, you have not method bodies in the time of this check - is it even possible?

I also don't like "Overrides" annotation (I don't like annotations at all, it's not Java) - is it even documented somewhere?

But mainly - thanks for your efforts! Can't wait for comments from others.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

I think doing it this without forcing a return type annotation is possible, it's just that the implementation will be a bit messier (I have to inject a fake return type that also carries which abstract def it's related to, so that it can be shown in an error, and we have no AST node for that) and I also prefer an explicit return type annotation to be in the source code for clarity, and eventually an Overrides annotation (which doesn't exist yet, so don't worry because it might never happen).

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

I think the worse case is this:

abstract class Foo
  abstract def foo : Nil
end

class Bar < Foo
  def foo
    1
  end
end

# Huh??
Bar.new.foo # => nil

The compiler will copy the Nil restriction over to the implementor, and it will trivially work because a Nil type annotation means anything can be returned and it will be ignored and nil will be returned instead.

In that case, yeah, we could force an explicit return type so that it's clearer... but since we'll be doing that for at least that case, I'd like it to be done for every case, which is more uniform and clear.

@petr-fischer

This comment has been minimized.

Copy link

commented Jul 10, 2019

...because a Nil type annotation means anything can be returned

So, why just not keep returning exactly what is returned (instead of enforced nil):

...
Bar.new.foo # => 1

Any bad consequences?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

So, why just not keep returning exactly what is returned (instead of enforced nil):

Because for example IO#wtite is not expected to return anything meaningful. If something implementing IO accidentally returns something other than nil then people can start relying on that value in their code, which is bad. I'd like it for subtypes to explicitly enforce the return type, and this is accomplished by an explicit return type.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

In fact I'd like there to be more and more type annotations over time. The feature about being able to omit types in method arguments is most of the time an anti-feature in my opinion. It's good for generic code, but most code is not generic.

Show resolved Hide resolved src/compiler/crystal/semantic.cr
@RX14

RX14 approved these changes Jul 23, 2019

@RX14

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

@asterite would you mind rebasing on master and squashing the fixup commits so this can be cleanly merged?

asterite added some commits Jul 9, 2019

Compiler: skip abstract def check for generic types
Let's tackle this in a future commit. It's tricky because of type
parameters but it can be done.
Deque actually doesn't implement Comparable
It could, but we can leave the implementation for later (or better:
make an Indexable comparable to any other Indexable)
Add a flag to skip abstract def checking
This is in case we got something wrong in the algorithm, so people can
still compile their code, or they don't want to bother upgrading their
code just yet.

@asterite asterite force-pushed the asterite:abstract branch from 30d353c to 9083fa9 Jul 23, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

@RX14 Done! I think the 6 commits that are left are good on their own.

@RX14 RX14 merged commit 719f49b into crystal-lang:master Jul 24, 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
@RX14

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Thank you, Ary <3

@asterite asterite referenced this pull request Jul 24, 2019

Merged

Release 0.30.0 #7990

@bcardiff bcardiff added this to the 0.30.0 milestone Jul 24, 2019

@bcardiff bcardiff added the topic:lang label Jul 24, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I guess this change is fine. But it's a potential source for annoying errors coming up in 0.30.0 when you have a large code base implementing abstract methods. It only shows one error at a time so you'll either need a fix-error-repeat cycle until all are fixed or find a different way to identify all malicious methods.

It's good to see that it can be disabled.
But did you consider adding this as a warning first in the next version?

With Crinja it actually showed me that the type restrictions on the abstract methods were wrong (which is getting fixed), but if they had been right, there would have been dozens of implementations that need a return type.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

I think we can do it as warnings.

My main issue with warnings is that they are opt-in, meaning that almost nobody will really take a look at them when upgrading. Then when we make the warning an error it will cause the same issue, code will break.

So before making this a warning I'd like warnings to be opt-out.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

The thing is that this is going to break quite a lot of code. And it can be difficult to fix when the missing return types are in shard dependencies. This will need fixes and updated versions for each of them. And this takes time. I've created PR's against some shards to fix this, but this is probably pretty common.

This breaking change has just come into the nightly build yesterday and the release is coming up in the next days. That's hardly enough time to properly identify and update breaking code. We should give maintainers more time (until 0.31.0) to do this without their code suddenly breaking.

(At the very least, the skip_abstract_def_check flag should be mentioned directly in the error message. This would improve things because users can directly skip these errors without having to read up on it. But that's still not optimal because automated builds will either break or need a specific customization just for this.)

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

You are right. Let's make it a warning and let's take our time to fix them in the next release. We should mention this prominently in the changelog and the release blog. I'll send a PR later today.

@bcardiff what do you think?

@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

I agree. Either mention the flag or be more conservative with the feature. We could make it opt-in for this release.

@maiha maiha referenced this pull request Jul 27, 2019

Merged

Fix examples 2019 07 #8003

oprypin added a commit to oprypin/crystal-chipmunk that referenced this pull request Aug 5, 2019

Compatibility with Crystal 0.30 (#4)
fix for abstract method change in crystal-lang/crystal#7956
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.