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

Misleading error for abstract method return type #9460

Closed
straight-shoota opened this issue Jun 11, 2020 · 4 comments · Fixed by #9461
Closed

Misleading error for abstract method return type #9460

straight-shoota opened this issue Jun 11, 2020 · 4 comments · Fixed by #9461
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic

Comments

@straight-shoota
Copy link
Member

This is extracted from #9454.

Since the changes in 0.35.0 to the return type of IO#write, lot's of errors like this pop up:

In /usr/lib/crystal/io/hexdump.cr:35:28

 35 | def write(buf : Bytes) : Int64
                               ^
Error: method must return Int64 but it is returning (Int64 | Nil)

The problem is that the indicated location is not where the error is. The method IO::Hexdump#write just happens to call #write on the wrapped io. And when there's an IO subtype where #write doesn't have a type restriction, that type causes an error here.
But instead, the error should be reported at the original location where the type restriction is missing.

Isolated example:

abstract class Foo
  abstract def write : Int64
end
 
class Bar < Foo
  def write : Int64 # Error: method must return Int64 but it is returning Nil
    Baz.new.write
  end
end
 
class Baz < Foo
  def write # The error should be reported here!
    nil
  end
end
 
Bar.new.write

Adding Int64 type restriction to Baz#write fixes the issue. Then the compiler error correctly points to Baz#write as the offender.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic labels Jun 11, 2020
@asterite
Copy link
Member

Just a note that the error has nothing to do with subclasses. Here's the smallest snippet that reproduces this:

abstract class Foo
  abstract def foo : Int32
end

class Bar < Foo
  def foo : Int32
    false
  end
end

Bar.new.foo

The bug is that the abstract def checking produces a warning, but the warning is only shown on the codegen phase (why???). But before reaching that the semantic error already triggers where the return type doesn't match the type of the body.

The solution is to report the warning as soon as it is detected, as it was in the past, before warnings were refactored and moved to the codegen phase. Thank you.

@straight-shoota
Copy link
Member Author

This snippet reproduces the faulty compiler behaviour. But from the user perspective it doesn't matter because it still points you to the location with an appropriate error message.

@asterite
Copy link
Member

There's something that's not clear here that I just found out: warnings are not shown if there are compiler errors. In my opinion that's plain wrong.

@asterite
Copy link
Member

Oh, well, I guess the first snippet is the one that makes sense. But to fix it, if warnings were reported in addition to errors you would immediately know what the problem is. I have a fix.

But then, I also think "abstract method" warnings should be errors... except that the logic to implement that is very hard so we might not be able to do that before 1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants