Skip to content
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

Enable erase unions with tags #3282

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

alfonsogarciacaro
Copy link
Member

  1. Allow building records with Erase attribute, compiling them as plain JS objects
  2. Likewise, remove the limitation of Erase unions where multiple cases with multiple were not allowed. For this, I introduced the tag argument
  3. Fix Type of Erased types #2914: Don't exclude erased types from Fable.AST.

Features 1 and 2 basically make possible what we already tried in the past as an opt-in.

The last feature is the trickiest one because several patterns were relying on erased types being Any in Fable.AST. I managed to fix it for JS/TypeScript but for other languages I'm still outputting Any until the tests are fixed. Another thing to note is I'm wrapping new erased unions with a type cast so the type info is not lost.

@dbrattli If you need the info from erased types, please add Python here and check which tests fail. @ncave Unfortunately I broke the Rust tests again 😞 could you please have a look?

NOTE: This has been split from #3279. Some previous discussion here.

@alexswan10k
Copy link
Contributor

alexswan10k commented Nov 24, 2022

Hi @alfonsogarciacaro , I had a quick look at this but not sure I have anything conclusive to add. A couple of observations:

Line 812 - hasAttribute Atts.erase
should probably now be
hasAttribute Atts.emit

Previously the Erase;Emit type would not output the entity at all, but now it is creating a trait (it thinks it is now a real interface). I imagine this might be because the behaviour of Any has changed.

I wonder if we could go through instances of Any in the FableToRust transform, and where Any was previously used to handle the erased scenario, we replace them with some kind of active pattern to any entity that has an emit attribute? I imagine part of the problem is Any's are not considered to be wrapped in a RC but managed types such as traits and records are (by shouldBeRefCountWrapped). We made a design decision to keep Emit dumb (and unwrapped), so you would add your own ref/deref/clone operators as needed in the template. This might explain the as_ref calls.

Sorry my time is a bit sparse at the moment, but I will try and dig in a bit further in the next few days.

@ncave
Copy link
Collaborator

ncave commented Nov 24, 2022

Allow building records with Erase attribute, compiling them as plain JS objects

@alfonsogarciacaro Hmm, previously Erase meant erase, not Plain.
Is it #2914 that necessitates this change of attribute meaning?
Can we perhaps introduce a different attribute to achieve the same thing?
Say EraseWithType or PlainJsObject (bad names, I know, just for example).

@alexswan10k The reason we can't just use Emit as Erase is sometimes we may want to emit types or other declarations (and sometimes we may want to erase them).

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.

Type of Erased types
3 participants