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

Support Negation and Not nodes. #35

Closed
faultyserver opened this issue Nov 10, 2017 · 7 comments
Closed

Support Negation and Not nodes. #35

faultyserver opened this issue Nov 10, 2017 · 7 comments
Labels
feature-request Any request for a new feature of the language. Includes both syntax and library features. good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful!
Milestone

Comments

@faultyserver
Copy link
Member

faultyserver commented Nov 10, 2017

Negation and Not are probably the most common unary operations that can be done. The parser is already set up and tested to parse unary operations properly, but there is no support in the interpreter for them. For example, running the simple program 1 + -1, will yield a "Compiler bug:" error for the - on -1, saying that Negation nodes are not yet supported. A similar error is raised for a program like !true.

Adding interpreter support for Negations and Nots will just require adding visit methods for them. Ideally, the methods for both of these nodes, as well as all future unary operations, will live under src/myst/interpreter/nodes/unary_ops.cr, to compliment the existing binary_ops.cr at the same level.

To actually implement these methods, the interpreter should evaluate the value property of the node, then call the appropriate unary method on it and push the result to the stack. There is no defined standard for what this method should look like, but since Myst allows multi-clause definitions, I think using ! for Not and - for Negation is sufficient, even if it slightly degrades performance.

To do the call to the appropriate method, there isn't a nice convenience method available from the interpreter. However, there is NativeLib.call_func_by_name, which provides the same functionality for NativeLib methods:

def call_func_by_name(itr, receiver : Value, name : String, args : Array(Value))
func = itr.__scopeof(receiver)[name].as(TFunctor)
Invocation.new(itr, func, receiver, args, nil).invoke
end

Just copy-pasting these two lines and replacing name should be sufficient for calling ! or - on the value. Alternatively, if you can come up with a nicer way for doing these calls, that would be great :)

Also, be sure to add some specs for these new features! There are already specs for parsing them, but specs for their interpretation should live in spec/interpreter/nodes/unary_ops_spec.cr. If you need some inspiration for some specs to include, you can look at the parser specs, though these are a little hard to read, since they're inside of a macro.

Hopefully that's a sufficient explanation to get started, but feel free to ask any questions if you have them :) Also, if you want to tackle this, be sure to add a comment or a 🎉 reaction so everyone can be aware!

@faultyserver faultyserver added feature-request Any request for a new feature of the language. Includes both syntax and library features. good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful! labels Nov 10, 2017
@faultyserver faultyserver added this to the Next milestone Nov 11, 2017
@faultyserver
Copy link
Member Author

faultyserver commented Nov 11, 2017

I'd like to see this in 0.2.0, but it's not necessary. I'm more inclined to keep it open for someone else to tackle as a first contribution :)

@faultyserver faultyserver modified the milestone: Next Nov 11, 2017
@ghost
Copy link

ghost commented Nov 12, 2017

Nobody yet, really ?
Ok, I will work on this.

@ghost
Copy link

ghost commented Nov 13, 2017

Hi @faultyserver, before I continue implementation, let me know if the current one seems good to you : https://github.com/rainbru/myst/blob/5e973c3786794f0be2addeab5d8cc26abeab5688/src/myst/interpreter/nodes/unary_ops.cr#L4-27

I'm really not sure of this current try. Got many type/cast errors and the negation of Var isn't yet implemented. Let me know if I'm on the right track.

@faultyserver
Copy link
Member Author

So, something to think about is how these could be used on types other than integers or floats. For example, I might have a custom type that I want to define a Negation for, and I could then write an expression like -%MyType{} to do that negation. In fact, pretty much any expression could be used as the value of a UnaryOp. Some other examples include -some_variable, -(a + b), and -(-(-a)) (a Negation of a Negation of a Negation).

It would be basically impossible to try to cover all of these cases inside of the visit(node : Negation) method, but thankfully, this is exactly what the AST and the visitor pattern are for!

Instead of trying to use Value.from_literal or create new ones with val, the more general solution is to use visit(node.value), then value = stack.pop to get the result of whatever the expression was. For example, in the expression -(1 + 2), the value from stack.pop would be a TInteger representing 3.

The rest of the method, where you're calling the - method on the result, looks good to me :)

The implementation of Not should be pretty much identical. visit(node.value), then call the ! method on the result.

@faultyserver
Copy link
Member Author

Something I just realized, though (which it looks like you've already noticed based on your comment about :float_subtract) is that there's no way to define multiple clauses for native methods.

For now, I would suggest changing the name of the - method you're calling to something like negate. The name is ugly, but at least it won't collide with the existing float_subtract definition. The same needs to be done for !. I would suggest changing it to just not.

Then, you can implement the new NativeLib methods and register them under the name negate. For example, for Float, you could duplicate this line and change it to

NativeLib.def_instance_method(float_type, :negate, :float_negate)

This method will need to be defined for Integer and Float, but should not be implemented for any other types.


Finally, for Not, there can be some default behavior for objects that don't define a not method. All Value classes have a truthy? method that returns the truthiness of the value (it is only false for TNil and TBoolean.new(false), so if the lookup of the not method on the value fails, you can fall back to using .truthy? instead. This would match Ruby's behavior. The result would look something like this:

result =
  if (not_method = self.__scopeof(value)["not"]?).as(TFunctor)
    # This code is only reached if `value` has a method named `not` defined.
    # As such, the invocation can be run to get the result.
    Invocation.new(self, not_method, nil, [v] , nil).invoke
  else
    # Otherwise, use the default behavior of checking the truthiness of the value.
    TBoolean.new(value.truthy?)
  end

stack.push(result)

That's a lot of information, but I hope it helps :) If you have more questions, feel free to ask them here, or in #help channel in the discord server

@ghost
Copy link

ghost commented Nov 13, 2017

Ok, it gives me new search paths, thanks! Will try to a abstract level up with a very generic visit(node : Negation) with many overloaded negate methods. Thanks again!

@faultyserver
Copy link
Member Author

I keep forgetting to tag PRs with closes #... :/

Anyway, this was implemented in #46. Thanks, @rainbru :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Any request for a new feature of the language. Includes both syntax and library features. good first issue An issue that provides a good intro to working with the Myst codebase. Be helpful!
Projects
None yet
Development

No branches or pull requests

1 participant