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

toLeopard: Desirable & satisfying traits #150

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

towerofnix
Copy link
Member

@towerofnix towerofnix commented Jul 2, 2024

Resolves #123. Related to leopard-js/leopard-mentors#4 and #149.

This PR implements the alternate proposal outlined in the second half of #123 (comment). Both PRs should be equally effective in generally fixing issues to do with surprise NaN in converted projects; however, we expect this one will lend to nicer and more intuitive reporter and input definitions. Either way, behavior-testing Scratch projects are much needed.

This code is all brand new, from July 2024. Thanks to @adroitwhiz for TypeScript tips!

@towerofnix towerofnix added compatibility Mismatch or currently unsupported language behavior fmt: Leopard Pertains to Leopard format (JavaScript) labels Jul 2, 2024
This commit includes (or should include) all the same NaN-handling
rules as researched and defined for pull request leopard-js#149, accounting
for almost all new logic / line additions in this commit.
itemOf() returns anything - just the value at that place in the
list, not casted by default - so new index rules say it should be
treated as a zero-cast number.

Of course this can cause behavior problems as indicated in leopard-js#153,
but in general casting to a number before subtracting is the
correct behavior. NaN always becomes -1 when an index is desired.
@towerofnix towerofnix marked this pull request as ready for review July 2, 2024 18:24
towerofnix added a commit to towerofnix/sb-edit that referenced this pull request Jul 2, 2024
@towerofnix towerofnix requested a review from PullJosh July 2, 2024 18:29
@towerofnix
Copy link
Member Author

@PullJosh We've requested review on both this pull request and #149, but our hope is that if you don't see any stand-out issues, we'll merge this one and not the other. They're opposing pull requests:

#149 is more conservative and uses very similar code language, so has a smaller diff, but it introduces conceptual baggage that we've seen is hard to explain and tricky to reason about. This PR (#150) includes all of the exact same meaning (or at least is meant to), and so if you want to get a sense ofr how the two PRs express that meaning differently, you can compare the opcodes whose definitions were in #149 with the same ops in this pull request.

This PR changes the language we use to represent input shapes, splitting it into "desired" and "satisfied" definitions, which look similar but have distinct meaning. That meaning is all described in the DesirableTraits and SatisfyingTraits enums. Although we had to edit literally every block definition, they are virtually all just different representations of the same meaning - we just think that meaning is expressed more clearly, now.

In this PR, the code at the bottom of blockToJS for helping the satisfied traits meet the desired traits is a bit more complicated. But we think it's quite easy to reason about if you give it a read through. We chose to represent the logic mainly in terms of what traits are desired: after all, we're about to fill in casting code if needed, because the job of this code is to make the satisfied code (variable based on the block) meet the desired code (constant for any given input).

@towerofnix
Copy link
Member Author

Also, we haven't created any proper behavior testing projects, yet—specifically for behavior to do with NaN (which is changed, in the same ways, in both pull requests). Those are coming soon™, and should be prepared before merging either PR!

We did test the project in leopard-js/leopard-mentors#4, which appears to be fixed the same way in this pull request as in the other. That was a good sign, LOL. Similarly, only a single change in the "compilesb3" snapshot test, not that it was automatically testing nearly any NaN-related cases, to begin with. (Those are the only cases intended to change with the merging of either PR.)

We also filed a couple follow-up issues #152 and #153 addressing particular concerns we noticed while updating all the opcodes' definitions for this PR.

I regret doing all the block updating work in one commit. If I'd thought better, I'd have converted all the definitions word for word identically to their current Leopard behavior, and only afterwards implemented the special NaN handling. Sorry this makes for a less nice-to-read diff! However, you should still get the idea from the diff pretty quickly: anything that doesn't involve NaN has a very simple conversion between the old style and the new one.

-satisfiesInputShape = InputShape.String;
+satisfiesTraits = [SatisfyingTraits.IsString];

-const string1 = inputToJS(block.inputs.STRING1, InputShape.String);
+const string1 = inputToJS(block.inputs.STRING1, [DesirableTraits.IsString]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Mismatch or currently unsupported language behavior fmt: Leopard Pertains to Leopard format (JavaScript)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toLeopard: Static number casting issues in equality operator + NaN leaking
1 participant