brownplt / pyret-lang

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

runtime.wrap doesn't work correctly for fractional numbers #1562

Open blerner opened 3 years ago

blerner commented 3 years ago

Consider the following expression (on current CPO): image-pinhole-x(point-polygon([list: point(0, 0), point(2.5, 0), point(32, 0)], "solid", "red")). It will evaluate to 11.5 -- but it's a broken 11.5. It's not an instance of Roughnum, but it is a raw JS number, so it renders as a raw JS number, and doesn't render as a fractional quantity. The problem seems to be here, in runtime.js:

    function wrap(v) {
      if(jsnums.isPyretNumber(v)) { return makeNumberBig(v); }
      else if(typeof v === "number") { return makeNumber(v); }
      ...
    }

The definition of jsnums.isPyretNumber then says

  var isPyretNumber = function(thing) {
    return (typeof(thing) === 'number'
            || (thing instanceof Rational ||
                thing instanceof Roughnum ||
                thing instanceof BigInteger));
  };

and makeNumberBig says

    function makeNumberBig(n) {
      return n;
    }

We use wrap all over the image library, and probably elsewhere too, so it seems quite odd to me that wrap doesn't actually wrap numbers correctly... I'm not sure exactly what the right fix should be, though, since I don't think wrap has enough information to know whether to make an exact number or a roughnum. And I don't know what exactly we ought to do in the image library, short of using jsnums.* everywhere instead of raw JS arithmetic, to ensure that we create rationals when we can do so... It seems reasonable that our image library has to discretize its answers to the pixel grid, so things like "what's the pinhole location?" ought to be a rational at worst, if not also forced to an integer on the pixel grid as well...

jpolitz commented 3 years ago

wrap should just be removed as vestigial.

It was necessary when PyretNumber, PyretString, etc were represented as objects, which hasn't been the case for years, and also when we tried to wrap functions more. There aren't any uses of .wrap in pyret-lang, so my best guess without doing a deep history/blame dive is that it got effectively removed in pyret-lang without its occurrences being removed in code.pyret.org and hence images.

jpolitz commented 3 years ago

I'm happy to do this in a standalone commit, wait until other image work you're doing is done, or whatever is more convenient.