Skip to content

Support anyref variable declartions as Wasm locals #958

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

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

jayphelps
Copy link
Contributor

@jayphelps jayphelps commented Nov 15, 2019

declare function external(a: anyref): anyref;

export function internal(a: anyref): anyref {
  // is an anyref
  const b = external(a);
  return b;
}

Right now if you try to use an anyref as a variable declaration it barfs with:

ERROR: concrete type expected
    at c.getTempLocal

This solves it by making the compiler aware that anyrefs can be stored as locals

Keywords: reference types, reference-types, gc

@jayphelps jayphelps force-pushed the anyref-locals branch 3 times, most recently from 92998d0 to f1d8f48 Compare November 15, 2019 23:17
@MaxGraey
Copy link
Member

It seems this same issue with public email: #863

@jayphelps
Copy link
Contributor Author

jayphelps commented Nov 15, 2019

@MaxGraey it looks OK now I think. But one of them is failing because I forgot to update the snapshots I think and I didn't notice that by default it doesn't run the tests with that features/reference-types enabled so I thought the tests were all passing but they were just skipped. I'm not sure how to enable it, but I'm looking.

@jayphelps
Copy link
Contributor Author

Ok figured out how to run it enabled:

ASC_FEATURES=reference-types npm run test:compiler rt/flags "features/reference-types"

Fixing the snapshots now.

@jayphelps
Copy link
Contributor Author

All green. 🎉

@MaxGraey MaxGraey requested a review from dcodeIO November 15, 2019 23:36
@dcodeIO
Copy link
Member

dcodeIO commented Nov 16, 2019

Nice find! For a bit of background: It worked for var since these become distinct locals, but not for const and let because those obtain shared locals (a temp for the lifetime of their scope).

@jayphelps
Copy link
Contributor Author

@dcodeIO

Made the NOTICE file change.

Nice find! For a bit of background: It worked for var since these become distinct locals, but not for const and let because those obtain shared locals (a temp for the lifetime of their scope).

Great to know! With that in mind, I modified the test to use all three: const, let, var so it hits those code paths.

@dcodeIO dcodeIO merged commit 4f4dc7a into AssemblyScript:master Nov 16, 2019
@dcodeIO
Copy link
Member

dcodeIO commented Nov 16, 2019

Perfect, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants