Skip to content

Variant constructor experiment#1413

Merged
Janther merged 22 commits intomainfrom
constructor-experiment
Feb 27, 2026
Merged

Variant constructor experiment#1413
Janther merged 22 commits intomainfrom
constructor-experiment

Conversation

@Janther
Copy link
Copy Markdown
Member

@Janther Janther commented Feb 12, 2026

this one started as a though experiment based on the repetitive pattern of checking an instance in order to create the correct wrapping object.

👎 We lose some typechecks as this solution is more general.
👎 We lose strict coverage when there is a particular variant for which we don't have a test. (this was fixed in 47b9a4a)

😐 The types for the constructors and the keys in the Map could use some work.
😐 YulStatement and Statement had to check manually for YulBlock and Block because it generated a cyclical dependency (I could have solved this by having the factory be called in a different file but that was a bit too much and the noise is not that big).

👍 Built size decreased by 5KB 😮

Update:
Instead of a for loop we can use a Map using the Slang class constructor as key for a Map and use that to find the corresponding AST constructor. We would lose the proper inheritance tree check that instanceof does. (slight performance increase in exchange for the instanceof match)

Also if we choose to use the class name as the key, we can use an object instead of a Map which would result in a performance improvement, but that would deviate from the current instanceof approach.
I'm open to feedback here. (slightly bigger performance increase in exchange for a much looser match)

@Janther Janther requested a review from fvictorio February 12, 2026 14:32
@Janther Janther force-pushed the constructor-experiment branch 4 times, most recently from bc3b4af to bc1f2cf Compare February 18, 2026 02:06
@Janther Janther force-pushed the constructor-experiment branch 3 times, most recently from d9a6fa9 to 3440d44 Compare February 23, 2026 11:25
@Janther Janther force-pushed the constructor-experiment branch from 1db4664 to 2459c48 Compare February 25, 2026 13:29
Comment thread src/slang-nodes/ArgumentsDeclaration.ts Outdated
) {
super(ast, collected);

if (process.env.NODE_ENV === 'test') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this is very painful. I'd rather lose the ability of measuring coverage than do this tbh.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not in the standalone code but it's on the transpiled one. I am planning on an improvement to test this without having the test code in the source file.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it became a whole mini project, but since the change is so big it just made sense that the proper testing would require that much care.

Copy link
Copy Markdown
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comment.

@Janther Janther force-pushed the constructor-experiment branch 2 times, most recently from 01bfaf1 to 14bfc08 Compare February 26, 2026 02:22
@Janther Janther force-pushed the constructor-experiment branch from a9cc2d3 to 1044131 Compare February 26, 2026 11:24
this.variant =
variant instanceof slangAst.Block
? new Block(variant, collected, options)
: createNonterminalVariant(variant, collected, options);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this is different. Why not have [slangAst.Block, Block] in the array above?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the creation of the createNonterminalVariant happens at the file level so is the first thing being executed, Block.ts depends on Statements.ts which depends on Statement.ts which calls Block at the very beginning.

it's different if it's inside a function but we want the call to createNonterminalVariant to be the first thing to happen when we load the file and then forget about it. I believe with having createNonterminalVariant declared in another file would solve this, but I would have to do it for each file to keep consistency just to solve that one case. which wouldn't save much file size compared to the amount of work put behind.

or maybe delay the execution of the function with an event 🤔 but that would made the correctness of the code depend on the js event loop.

Here we only have one exception which is that for that one case we do a normal check which seemed for me a good compromise.

@Janther Janther merged commit ea29a8f into main Feb 27, 2026
7 checks passed
@Janther Janther deleted the constructor-experiment branch February 27, 2026 12:26
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.

2 participants