evaluator: Add index and dot expression evaluation by juliaogris · Pull Request #39 · evylang/evy

juliaogris · 2022-10-18T03:34:49Z

Add index and dot expression evaluation in evaluator, similar to binary
expression evaluation.

Change Array.Elements and Map.Order to pointer. This is another preparatory
step for proper array and map manipulation so that two variables may
reference the same array or map and if one gets updated the other one will
get updated too.

Rework scoped Value updates such that instead of replacing the value in the
scope on assignment with scope.set(name, newValue), we now update the Value
field with scope.get(name).Set(newValue)

The first three commits are covered by PRs #37 and #38 .

github-actions · 2022-10-18T03:37:25Z

firebase-deployment: https://evy-lang--39-zvdfxh68.web.app (0237bfa)

juliaogris · 2022-10-18T03:37:33Z

pkg/parser/expression_test.go

 		`print 1 (len "abc") 2`:       "print(1, len('abc'), 2)",
 		`print (len "abc") 2`:         "print(len('abc'), 2)",
 		`print (len "abc") (len "x")`: "print(len('abc'), len('x'))",
+		`print s[1]`:                  "print((s[1]))",


This was a nil panic before

juliaogris · 2022-10-18T03:37:59Z

pkg/parser/parser_test.go

+		`
+m := {name: "Greta"}
+s := name
+print m[s]`: "line 3 column 6: unknown variable name 'name'",


nil panic before fix.

camh-

🍃 LGTM. A few comments, but feel free to fix, commit and merge.

camh- · 2022-10-19T11:34:07Z

pkg/evaluator/evaluator.go

 		return val
 	}
+	if decl.Type() == parser.ANY_TYPE {
+		val = &Any{Val: val}


If e.Eval(...) above returns an Any, should you just use val as is?

If I remove these added lines I get a test failures:

--- FAIL: TestAssignmentAny (0.00s) evaluator_test.go:174: want != got "false 0\n0 0\n🦊 0\n" "false 0\nfalse 0\nfalse 0\n"

The zero value for any is false; the inferred decl might by anything, but in either case it does not return the wrapped any type it seems but the bare type.

If you search for &Any{.... it doesn't seem to be created anywhere in the evaluator.go file.

OH! I think you are suggesting I should just add it to the main switch.

hmmm. not sure actually

add a parseAnyLiteral ?
The wrapped value could hold an error so we want to surface that I think.

also: there is no AST node for Any...
I think I need to leave this as is.

I just meant if eval could return an any will you end up wrapping an any in an any?

camh- · 2022-10-19T11:43:16Z

pkg/evaluator/value.go


+func (m *Map) Set(v Value) {
+	if m2, ok := v.(*Map); ok {
+		m.Pairs = m2.Pairs


Just *m = *m2 should do i think (instead of assigning individual elements) - and you could do that for all the Set() methods if you like

camh- · 2022-10-19T20:53:12Z

pkg/evaluator/evaluator.go

+	case *Map:
+		index, ok := index.(*String)
+		if !ok {
+			return newError("expected string for map index, found " + index.String())


hmmm, isn't index here an empty string or something from two lines above? Shadowing vars is confusing. I was also confused about left which I thought was a Node, but later realised it was shadowed by the type switch. I guess its ok, I'm not sure what other name to give it that makes sense.

But, shouldn't you be printing the type of the incorrect map index? as in "expected , got "?

Edit: checked the spec, and indeed index will be an empty string in this error message.

ok, I've renamed this to strIndex and also renamed left := left.(type) to l := left.(type).
I've added an extra fixup commit on top doing the rename from the previous evalBinaryExpr function for consitency.

camh- · 2022-10-19T21:00:55Z

pkg/evaluator/evaluator_test.go

+		"print m.name":    "Greta",
+		`print m["name"]`: "Greta",
+		`s := "name"
+		print m[s]`: "Greta",


Should there be a test case for looking up a key not in the map? I can't remember what the semantics of that is? Returns zero value like Go? (as an aside, what's the zero value of any? doesn't seem to be in the spec)

ok, I see from below value.go that a bad index is an error. leaving this comment for the any question.

🍍

ok, good point. I've added a test case. Will add more when implementing has and del functions.

I wonder if I should return the zero value....

sorry, no good at not answering.

Fix IndexExpression.Type() for indexed string, e.g. "abc"[1]. We indiscriminately returned left.Type().Sub, which is only correct for arrays. For strings the indexed type should also be string.

Add missing early return on index expression parse error. This resulted in a "dereferenced nil pointer" panic at run time.

Rework scoped Value updates such that instead of replacing the value in the scope on assignment with scope.set(name, newValue), we now update the Value field with: val := scope.get(name) val.set(newVal) This is a preparatory step for index and dot expressions which also update the "internals" of a Value. As a fall-out of this step we had to extend the Value interface with a `Set(Val)` method. We also had to remove the bool constants TRUE and FALSE, as the internals of these constants got updated. `scope.getScope (name)` is now not needed any longer and removed. Additionally introduce an `Any` type that implements Value and wraps a value so that we can update an any value from one type to another, e.g. Bool to String.

Change Array.Elements and Map.Order to pointer. This is another preparatory step for proper array and map manipulation so that two variables may reference the same array or map and if one gets updated the other one will get updated too. In go maps are references and natively support this property, however slices may get allocated new memory and are not guaranteed to keep the same reference and appended.

Add index and dot expression evaluation in evaluator, similar to binary expression evaluation.

Rename local variable in evalBinaryExpr for consistency with new evalIndexExpr: avoid variable shadowing when type casting in switch statement.

juliaogris commented Oct 18, 2022

View reviewed changes

camh- approved these changes Oct 19, 2022

View reviewed changes

juliaogris added 2 commits October 20, 2022 09:27

parser: Fix IndexExpression.Type() for indexed string

7e711cf

Fix IndexExpression.Type() for indexed string, e.g. "abc"[1]. We indiscriminately returned left.Type().Sub, which is only correct for arrays. For strings the indexed type should also be string.

parser: Fix missing early return on index error

83658de

Add missing early return on index expression parse error. This resulted in a "dereferenced nil pointer" panic at run time.

juliaogris force-pushed the eval-index branch from 3548b60 to e564415 Compare October 19, 2022 22:28

juliaogris added 2 commits October 20, 2022 09:45

juliaogris force-pushed the eval-index branch 2 times, most recently from ba173e9 to f27370f Compare October 19, 2022 22:53

juliaogris added 2 commits October 20, 2022 10:00

evaluator: Add index and dot expression evaluation

7c3c35f

Add index and dot expression evaluation in evaluator, similar to binary expression evaluation.

evaluator Rename local variable for consistency

0237bfa

Rename local variable in evalBinaryExpr for consistency with new evalIndexExpr: avoid variable shadowing when type casting in switch statement.

juliaogris force-pushed the eval-index branch from b09dda1 to 0237bfa Compare October 19, 2022 23:01

juliaogris merged commit f8b2aca into master Oct 19, 2022

juliaogris deleted the eval-index branch October 19, 2022 23:03

Conversation

juliaogris commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camh- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juliaogris commented Oct 18, 2022 •

edited

Loading

github-actions bot commented Oct 18, 2022 •

edited

Loading