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

Hitting an undefined class gives a debugger with misleading message "#new was sent to nil" #2776

Closed
macta opened this Issue Mar 8, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@macta
Copy link
Contributor

macta commented Mar 8, 2019

When you save a method that references an as yet undefined class - there is now an option to save is as undeclared (the same applies when using the Opal compiler - it can be configured to do this automatically as well).

This is a great enhancement, that lets you code "in the now" without being forced down another path you aren't ready for.

However - when you execute the code and hit that undefined class (perhaps due to something like: MyUndefinedClass new" the debugger shows the messsage "#new was sent to nil".

While this might be technically correct - its not very helpful and I'm sure we can do better. In my mind, this is an undefined class (not the same as nil), and I wonder if it is stored in the bytecode as such (or could be) so that we could present the more informative message "#MyNewClass is Undefined".

Everything else is then perfect, as we have already fixed the debugger so that create will let you create a new class an continue.

So its just the message - and then it really makes TDD awesome! (its still pretty awesome now - in Exercism, when you run a test, you can click Create, to get a new class, continue, then get a debugger and click Create again to define a new method - its very fluid).

Tim

@bencoman

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 8, 2019

@macta

This comment has been minimized.

Copy link
Contributor Author

macta commented Mar 8, 2019

If the byte code can’t handle it - then I guess we can marry it up with the source to identify the variable name and show the right message. At the moment it’s just misleading - I didn’t mean to save null, I saved an undeclared variable that has a name. Anyway, it sounds doable somehow

@bencoman

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 9, 2019

[Raw] Tab shows... literal1 "#MyUndefinedClass->nil"

I wonder if that comes from...
RBVariableNode>>binding
^self propertyAt: #binding ifAbsent: [nil].

Potentially useful methods...
RBVariableNode>>hasIncompleteIdentifier
RBVariableNode>>isUndeclared

I notice that RBVariableNode has 7 subclasses.
Perhaps it would help to have a subclass RBUndefinedGlobalOrVariableNode.

@macta

This comment has been minimized.

Copy link
Contributor Author

macta commented Mar 9, 2019

Actually Ben you are right - in the exception, in the context (thisContext) there is the sender of the message , which has a method (CompiledMethod) and you could then traverse the AST to find the method name (#new) who is sent as a message to a node where #isUndeclared is true.

Its a bit of a mouthful, but I wonder if there is a more immediate way - as you mention the raw view it seems to show it - so sending #literals to the method in question does give an array of them - and you can select: [ :i | i isKindOf: UndeclaredVariable ]. But if there was more than one - not sure you can marry that up to the error...

And then the penny drops - thisContext sourceNodeExecuted give the node you were executing.
so: thisContext sourceNodeExecuted nodesDo: [:n |
(n isVariable and: [ n isUndeclared]) ifTrue: [ ^n name ] gives you the name of the undeclared variable you hit.

But not quite sure how to plug that together yet.

@macta

This comment has been minimized.

Copy link
Contributor Author

macta commented Mar 9, 2019

Ok - I know how to do it, and I think it simplifies my previous class creation submission (doing it properly often simplifies things)

@macta

This comment has been minimized.

Copy link
Contributor Author

macta commented Mar 11, 2019

@Ducasse - I thought putting the word fix in the pr would close the issue automatically? Given this is merged, should this have closed automatically?

@bencoman

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 11, 2019

use the phrase "Fixes #123" or "Fixes: #123" in your pull request "description" or "commit message"
not just in a comment
https://help.github.com/en/articles/closing-issues-using-keywords

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.