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.

Sign up for GitHub

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

Jump to bottom

Context: show more helpful message for never called method #4805

Merged
merged 2 commits into from
Aug 14, 2017
Merged

Context: show more helpful message for never called method #4805

merged 2 commits into from
Aug 14, 2017

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Aug 7, 2017 •
edited
Loading

Close #4158
Close #4804

We cannot get a context from the method which is never called. But current implementation yields an error in such a case, this behavior seems wrong.

This fixes it to show additional information when user tried to get a context from never called method.

$ cat foo.cr
class Foo
  def foo(env)
    puts env
  end
end

$ # Old:
$ crystal tool context -c foo.cr:3:10 foo.cr
BUG: `env` at foo.cr:2:11 has no type
0x10b72f3f3: *raise<String>:NoReturn at ??
0x10ba34c6c: *Crystal::Arg@Crystal::ASTNode#type:Crystal::Type+ at ??
0x10c246f85: *Crystal::ContextVisitor#visit<Crystal::Def+>:Bool at ??
0x10c2470f9: *Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::ContextVisitor>:Nil at ??
0x10c247f00: *Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::ContextVisitor>:Nil at ??
0x10c2485fb: *Crystal::ASTNode+@Crystal::ASTNode#accept<Crystal::ContextVisitor>:Nil at ??
0x10c241116: *Crystal::Command#tool:(Bool | IO::FileDescriptor | Nil) at ??
0x10b78dc0f: *Crystal::Command#run:(Bool | Crystal::Compiler::Result | IO::FileDescriptor | Nil) at ??
0x10b75eb86: main at ??

Error: you've found a bug in the Crystal compiler. Please open an issue, including source code that will allow us to reproduce the bug: https://github.com/crystal-lang/crystal/issues

$ # Now:
$ ./bin/crystal tool context -c foo.cr:3:10 foo.cr
no context information found (never called method has no context anytime)

@RX14
Copy link
Contributor

RX14 commented Aug 7, 2017

Are you sure you want to mention #4185?

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 7, 2017 •
edited
Loading

sorry, it's my mistake.

@makenowjust
Context: show more helpful message for never called method
f4c3ba8 
Close crystal-lang#4158
Close crystal-lang#4804

We cannot get a context from the method which is never called. But
current implementation yields an error in such a case, this behavior
seems wrong.

This fixes it to show additional information when user tried to get a
contex from never called method.
@makenowjust makenowjust force-pushed the fix/crystal-context/detailed-message-for-nerver-called-method branch from 6a80e92 to f4c3ba8 Compare August 7, 2017 12:40
@makenowjust
Copy link
Contributor Author

fixed.

RX14
RX14 requested changes Aug 7, 2017
View reviewed changes
spec/compiler/crystal/tools/context_spec.cr Outdated
@@ -407,4 +407,15 @@ describe "context" do
Foo.foo 100
), "self", "a"
end

it "can't get a context from never called method" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"can't get context from uncalled method"

src/compiler/crystal/tools/context.cr Outdated
@@ -155,7 +164,11 @@ module Crystal
end

if @contexts.empty?
return ContextResult.new("failed", "no context information found")
if @found_untyped_def
return ContextResult.new("failed", "no context information found (never called method has no context anytime)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "no context information found (methods which are never called cannot have a context)"

Copy link
Contributor

@bew bew Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RX14, good but I'd slightly change the end:
s/cannot have a context/don't have a context ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Thank you @RX14 and @bew.

makenowjust added a commit to makenowjust/crystal that referenced this pull request Aug 7, 2017
@makenowjust
Fix message and spec
9516ad6 
crystal-lang#4805 (comment)
crystal-lang#4805 (comment)
@makenowjust makenowjust force-pushed the fix/crystal-context/detailed-message-for-nerver-called-method branch from 9516ad6 to 0eaa19e Compare August 7, 2017 22:05
@makenowjust
Copy link
Contributor Author

ping. This seems ready to merge.

@RX14 RX14 merged commit ac88068 into crystal-lang:master Aug 14, 2017
@RX14 RX14 added this to the Next milestone Aug 14, 2017
@makenowjust makenowjust deleted the fix/crystal-context/detailed-message-for-nerver-called-method branch October 2, 2017 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bew bew bew left review comments

@RX14 RX14 RX14 approved these changes

@jhass jhass jhass approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.

BUG: has no type Tool context BUG: has no type
4 participants
@makenowjust @RX14 @jhass @bew