brownplt / pyret-lang

The Pyret language.
Other
1.06k stars 106 forks source link

Inconsistent error terminology for record-annotation mismatches #1740

Closed blerner closed 1 month ago

blerner commented 1 month ago

Consider a simple program:

type Point2D = { px :: Number, py :: Number }

fun next-pos(p :: Point2D) -> Point2D:
  { px : p.x + 1 }
end

Then consider two bad uses of that function, and the errors we get on the command line or in CPO:

Questions:

  1. Do we want the terminology to be "record" or "Object"?
  2. We can't fix things at the command-line, since we don't have the render-fancy-reason available there. But is it worth trying to special-case this error message and see if we can't get it to show "The Point2D annotation was not satisfied..."? (I can get this to work when Point2D is defined in the same file as it's being used, but I can't seem to get my localhost instance to read a shared gdrive file, so I can't test yet whether it works across files...which is where it's most useful.)
blerner commented 1 month ago

(@jpolitz , @shriram , would like your input here if you have time)

blerner commented 1 month ago

@jpolitz here's a thought to fix question 2 above:

  1. Decorate PRecordAnn in runtime.js with an optional name field (and ditto PTupleAnn)
  2. In PRecordAnn.prototype.check, if there is a name field then use it, else use "record" (or "Object", whatever we decide)
  3. In anf-loop-compiler, augment compile-ann to take in a Option<String> name, that we can supply in a-type-let definitions (possibly some other analogous locations) and leave out in other situations.

The idea being, we'd wind up compiling type Point2D = {px:: Number, py:: Number} as

var Point2D## = R.mRA([ "px", "py" ],
                    [ L[7], L[8] ],
                    { "px": $type$Number1, "py": $type$Number1 },
                    "Point2D"
);

so that when it's used in an annotation-checking position, the runtime can say "oh, I'm a named annotation, I can give a better error message here".

I think this approach will work across modules and not require AST access (i.e. it'll work at the command line). The only downside I can think of is that if a type is aliased when imported or exported, the error message will be in terms of the original defining name.

I've just pushed an implementation of this idea to the PR above, so we can try it out.