Fable icon indicating copy to clipboard operation
Fable copied to clipboard

Enable erase unions with tags

Open alfonsogarciacaro opened this issue 3 years ago • 4 comments
trafficstars

  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 #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.

alfonsogarciacaro avatar Nov 23 '22 15:11 alfonsogarciacaro

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.

alexswan10k avatar Nov 24 '22 17:11 alexswan10k

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).

ncave avatar Nov 24 '22 17:11 ncave