Skip to content

Fix subs test#57

Merged
jeaye merged 1 commit intojank-lang:mainfrom
jaidetree:fix-subs
Jan 25, 2025
Merged

Fix subs test#57
jeaye merged 1 commit intojank-lang:mainfrom
jaidetree:fix-subs

Conversation

@jaidetree
Copy link
Contributor

Previously, when building a release version of the tests, shadow-cljs defaults to :advanced optimizations. For most apps, the default behavior is ideal and you get really well optimized code.

Code like ("abcde" nil 1 2) gets inlined to "abcde".substring(1,2), but in our tests (subs nil 1 2) would get inlined to null.substring(1,2). Given the Google Closure compiler shadow-cljs uses, it recognizes that is invalid code and just outputs null. This results in code that is effectively (thrown? nil) which raises the error about receiving nil instead.

This sets the :optimizations to :simple in the release build. While it still produces decently optimized code, it doesn't factor out our intentionally invalid calls which is ideal for writing tests.

Previously, when building a release version of the tests, shadow-cljs
defaults to :advanced optimizations. For most apps, the default behavior
is ideal and you get really well optimized code.

Code like `("abcde" nil 1 2)` gets inlined to `"abcde".substring(1,2)`,
but in our tests `(subs nil 1 2)` would get inlined to
`null.substring(1,2)`. Given the Google Closure compiler shadow-cljs
uses, it recognizes that is invalid code and just outputs `null`. This
results in code that is effectively `(thrown? nil)` which raises the
error about receiving nil instead.

This sets the :optimizations to :simple in the release build. While it
still produces decently optimized code, it doesn't factor out our
intentionally invalid calls which is ideal for writing tests.
@dgr
Copy link
Contributor

dgr commented Jan 25, 2025

Nice! Thanks for tracking this down, @jaidetree.

@jeaye jeaye merged commit 6830913 into jank-lang:main Jan 25, 2025
3 checks passed
@jeaye
Copy link
Member

jeaye commented Jan 25, 2025

Great job, @jaidetree!

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