Reinstate @ decorator for unary ops.

[chrisseaton]

Fixes #4.

It's debatable whether getName() and getDecoratedName() should return different things here. My thought is that -@ is the name of the method. It's not decorated - that is the name. Anything else is a syntactic shortcut.

Also a note about how to use Jay, as I found it a little confusing.

[enebo]

Chris, I have 2 comments on this PR. I see you updated Ruby18Parser.java to contain setValueNode instead of setValue. You should revert that set of changes since setValueNode was replaced by setValue (although the .y file was not updated).

The second name is more about what should be returned by getName() and getDecoratedName(). In how they have been used I would say getName() should return '-@' and getDecoratedName() should return '-'. Even this recommendation worries me since I think in netbeans we use getName() for length/name of identifier for things like instant renaming. I loathe the idea of making a third name method but I think there are three cases here:

1. get name + any sigil for lexical purposes (like rewriting the argument node)
2. get name sans sigil for lexical purposes (for things like variable rename)
3. get name for semantic purposes (like -@ instead of -)

It is possible the getName,getDecoratedName concept is flawed. Perhaps I need getName() and getSigil() but I think even this would still need a third method for semantic name. So having rambled through this I wonder if we should have getSemanticName()? So then the real recommendation would be to:

getName -> '-'
getDecoratedName -> '-'
getSemanticName -> '-@'

This however requires considerable change to CallNode so perhaps for now use my original recommendation of getname -> -@ and decoratedname -> -. Expect this to change later to accommodate difference between semantic and lexical needs though (unless you have some other thoughts on how this should be addressed).

[chrisseaton]

I'm not sure how I managed to change Ruby18Parser.java without changing Ruby18Parser.y. I'll look at that again.

I follow your reasoning, but I think your solution for now, getName -> -@ and getDecoratedName -> -, has to be the least intuitive of them! The decorated version returns the shorter name!

However if you think that's best, I'll implement that. I'm just keen on being able to detect unary ops however it's done.

[enebo]

yeah I am mostly form-fitting here. Perhaps Unary calls can become a specialized node type. That would then still return '-' for both name and decorated name but it would be easily detectabe?

[chrisseaton]

This just removes the unary @ in getDecoratedName. I'm not terribly familiar with Ruby - is there a risk that we remove the @ from a method that isn't expecting to have it removed?

I think your last commit recreated the 1.8 parser.

[enebo]

Ok Chris, I was just thinking about how to solve this nicely and I think perhaps we just need to make the names clearer. I think there are only two method named: getName() and getLexicalName().

getName() represents semantically what is needed for making a runtime. We dispatch to @- so getName() will return that.

getLexicalName() returns what is needed to rewrite the AST back to source essentially. In this case, it would just return '-'.

Note the case of unary minus is different than most:

@foo

getName() -> 'foo'
getLexicalName() -> '@foo'

As you can guess I just plan on renaming getDecoratedName to getLexicalName. At the time I made the method I did not realize that it had cases like unary minus and just went with the wrong concept in my head. Renaming lexical to me makes this much more intuitive. What do you think?

[chrisseaton]

Yes, those names seem nice and clear and will solve my use case.

My PR patch has the bug that it adds the sigil to all unary ops, such as ~. There needs to be a whitelist to stop it doing this. I think it's just + and - that have it when unary.

Let me know if you want my patch updating in some way, or feel free to discard it or whatever, I'm not attached.

[chrisseaton]

Is this the right way around? You said that getName() would return @-, but then you say getLexicalName() -> '@foo'? Those seem to be opposite? How is unary minus different to most? In Numeric http://www.ruby-doc.org/core-2.0/Numeric.html they're both marked as being @ something.

[enebo]

Ah I think I see what I did to confuse this... For the def @- then the lexical name is @- and the same as @foo. I was thinking that at a callsite the lexer was supposed to provide '@-' where then that would be the name but '-' would be the lexical name.

[chrisseaton]

Yeah, it's not simple is it. What's more you can do 14.-@(), where both getName and getLexicalName need to return -@, so we need to store how it was found, not store - and then sometimes add @.

[enebo]

Actually your example made me realize that unary as a call op is special and should be made a specialized node. Your example number.-@() is not special and it just an ordinary call. So I think we need UnaryCallNode or something like that? The special class makes getName and getLexical name require no special logic then.

[enebo]

I should have stated that number.-@() is doing nothing special lexically...

[chrisseaton]

A UnaryCallNode does sound good - we already have several nodes for several different types of call so it won't break any assumptions that all calls should be homogenous.

[chrisseaton]

I'm proposing https://github.com/chrisseaton/jruby-parser/blob/issue-8-proposed-specs/spec/parser/calls_spec.rb as a set of specs to work against for calls.

It says that only for the operators - and +, which are the only ones decorated with @, we use a UnaryCallNode. It returns the decorated version for the name, and the undecorated version for the lexical name. If someone explicitly calls the unary operator via x.-@ then we give them a normal CallNode, not a UnaryCallNode, so UnaryCallNode is quite specific.

Note that in writing these specs I'm not sure about our not behaviour, for parsing not x:

1.8: (RootNode, (NewlineNode, (NotNode, (VCallNode:x))))
1.9: (RootNode, (NewlineNode, (CallNode:not, (VCallNode:x), (ArrayNode))))
2.0: (RootNode, (NewlineNode, (CallNode:!, (VCallNode:x), (ListNode))))

1.8 seems right, but 1.9 replaces it with a call to the not method. Was there ever such a method? I can't find it in the docs. 2.0 replaces it with a call to !. Is that new behaviour? There don't seem to be keyword docs for 2.0, but the keyword docs for 1.9 say specifically say that not cannot be overridden.

[chrisseaton]

I've made a right mess of the issues for this bug. This is a pull request, that we're no longer going to merge, so I'll close it and continue discussion on the original bug #4.