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

Compiler ignores call to undefined method after a NoReturn call #6100

Open
RX14 opened this issue May 15, 2018 · 10 comments
Open

Compiler ignores call to undefined method after a NoReturn call #6100

RX14 opened this issue May 15, 2018 · 10 comments

Comments

@RX14
Copy link
Contributor

RX14 commented May 15, 2018

def defined_method : NoReturn
  raise "no"
end

undefined_method(defined_method)

The above example compiles and runs despite there being a call to an undefined method in the body. This is annoying when undefined_method is actually intended to be a macro call and can lead to renaming a macro definition, but not renaming a usage silently succeeding where it should fail to compile.

@bew
Copy link
Contributor

bew commented May 15, 2018

Even more reduced: foo raise "bar"
Which compiles and gives the exception (https://carc.in/#/r/42r0)

@SlayerShadow
Copy link

SlayerShadow commented May 15, 2018

@RX14, @bew, why do you think that it is a bug? Ruby has the same behaviour because code launches from right to left.

Just by nature, Crystal compiler can optomize your code to this:
defined_method
(or raise bar in @bew's example) because it knows that left part will never run.

Try this:

class Some
  def initialize
    @foo = 3
    @bar = "xxx"

    @foo + @bar
  end
end

p :success

This code compiles correctly and even launches. But if we change last line to Some.new, we will take an error.

@SlayerShadow
Copy link

SlayerShadow commented May 15, 2018

The undefined method is reproducible if we leave some "door" to make launch undefined_method() action possible:

def defined_method : Void
  if ARGV.size > 0
    raise "no"
  end
end

undefined_method(defined_method)

Always shows Error in app/temp.cr:7: undefined method 'undefined_method'. Point - we do not describe else behaviour. Again, if we add else and put raise there, we catch the raise's exception.

@RX14
Copy link
Contributor Author

RX14 commented May 15, 2018

Whether it's a bug or a feature request is moot: I explained why this behaviour is undesirable above.

@straight-shoota
Copy link
Member

Using return fails at least:

def foo
  bar return # Syntax error: void value expression
end

foo

@SlayerShadow
Copy link

SlayerShadow commented May 16, 2018

@RX14, I did not say that it is a bug or a feature, i just asked "why".

My research led to the hypothesis that LLVM, or GCC, or linker can remove unused functions due to optimization reasons (source 1, source 2). It is nice and it is bad.

  • SlayerShadow: I wrote a big code. Let's try to run an empty page without any logic - just classes initialization and see it has no bugs. Crystal! Compile!
  • Crystal: Yes! It is working code! Compiled successfully.
  • SlayerShadow: Really?.. Well, now let's initialize a little class' instance to check if my world launches (added SuperWorld.new). Crystal! Compile again!
  • Crystal: BOOM! Argument Error! Instance variable should be UInt8 but input was Int32: here, here, here, here. Additionally here and here. Also here, here and here. Another over 9k little errors.
  • SlayerShadow: o_O

Maybe it is better to at least notify user that there is unused code like -Wunused.
If it can be fixed somehow else (maybe look-up table through whole app to search and point used and unused functions), it can move Crystal to one step for create a send action which could call any method.

@RX14
Copy link
Contributor Author

RX14 commented May 16, 2018

@SlayerShadow it's impossible to compile a function which isn't called in crystal. That's a limitation of the typing algorithm (nothing to do with llvm, gcc, linker, etc). That's never going to change. See #4402. And send can't be implemented for the exact same reason.

What i'm thinking is essentially an enhancement: at least look up the method names to see if they exist in scope, even if we can't type them.

@SlayerShadow
Copy link

@RX14, thank you for detailed answer.

@asterite
Copy link
Member

asterite commented May 23, 2018

It's a feature. If the type is NoReturn, the call can't be made, so no overload will ever match it. Maybe we can check if at least one method exists with that name, and give an error if not. I personally don't find this a problem at all. Why is it a problem?

@RX14
Copy link
Contributor Author

RX14 commented May 23, 2018

@asterite I explained up above (and I agree on your method of fixing it): the undefined_method was actually previously a macro and was therefore used and expanded. Then you rename the macro definition and the compiler silently ignores the macro call (assuming it's a method call) because of the NoReturn.

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

5 participants