brownplt / pyret-lang

The Pyret language.
Other
1.07k stars 109 forks source link

Using `spy` with `var` produces strange error message #1342

Closed ZacharyEspiritu closed 6 years ago

ZacharyEspiritu commented 6 years ago

Attempting to use spy with an assignable variable produces a strange error message:

var x = 1
spy: x end
screen shot 2018-06-12 at 10 51 59 am

The docs don't say whether or not spy should work with assignables, but since print works with var variables, I expected spy to work as well.

The following program also produces a similar error whether or not f is applied to anything:

fun f(x):
  var y = 1
  spy: y end
end
screen shot 2018-06-12 at 10 56 49 am 1
blerner commented 6 years ago

Oh that is a silly bug indeed. You can avoid it for now by saying spy: any-name: x end, where any-name is a valid Pyret identifier to be used as a label for the expression to be spied upon.

To fix it... there are two possibilities. First, we could change ast.arr line 1216 to not assert the is-s-id annotation on its argument, because it's simply false for s-id-vars. But...that annotation served its purpose, and showed us a bug in our code, so I don't want to eliminate it. Second, we could simply handle this in the parser itself when we construct the AST, by changing the data definition

data SpyField:
  | s-spy-name(l :: Loc, name :: Expr%(is-s-id)) with:
    method label(self): "s-spy-name" end,
    method tosource(self): self.name.tosource() end
  | s-spy-expr(l :: Loc, name :: String, value :: Expr) with:
    method label(self): "s-spy-expr" end,
    method tosource(self): 
      PP.nest(INDENT, PP.str(self.name) + str-colonspace + self.value.tosource())
    end
...

to

data SpyField:
  | s-spy-expr(l :: Loc, name :: String, value :: Expr, implicit-label :: boolean) with:
    method label(self): "s-spy-expr" end,
    method tosource(self): 
      if self.implicit-label: self.name.tosource()
      else: PP.nest(INDENT, PP.str(self.name) + str-colonspace + self.value.tosource())
      end
    end
...

and just construct s-spy-exprs in parse-pyret.js, where implicit-label is true for the shorthand form spy: x end, and false for the longer form spy: some-name: x end.

(A third possibility is to add a third constructor, s-spy-var, that gets constructed during name resolution...but that's kinda brittle to me, so I don't want to do that.)

Want to try fixing this?

sorawee commented 6 years ago

so I don't want to eliminate it

But the second solution doesn't need s-spy-name anymore. You in fact want to eliminate the entire thing :)

blerner commented 6 years ago

Eliminating the constructor altogether is fine -- it's impossible to construct one incorrectly ;-) But keeping the constructor and eliminating its protections would make things less robust overall.

ZacharyEspiritu commented 6 years ago

Working on this now off of @blerner's modification of SpyField—will update when I get something to work!