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

Disable -bs-cross-module-opt for tests #7071

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Oct 4, 2024

Runtime + tests are currently compiled with -bs-cross-module-opt.

Remove this option so that the way the tests are compiled more closely matches the way real-world code is compiled.

FIXED: When removing this option for the tests, I see some expected changes and some rather surprising ones, like the function Obj.magic suddenly appearing in the JS output.

tests/tests/src/gpr_4069_test.js Outdated Show resolved Hide resolved
tests/tests/src/jsoo_400_test.js Outdated Show resolved Hide resolved
@cknitt
Copy link
Member Author

cknitt commented Oct 4, 2024

I think this may be related to the primitives changes? @cometkim @cristianoc

@cristianoc
Copy link
Collaborator

I think this may be related to the primitives changes? @cometkim @cristianoc

Obj.magic is defined as "%identity". If it was defined differently before, then surely the change is the cause for the difference in the generated code.

@cknitt
Copy link
Member Author

cknitt commented Oct 4, 2024

There is now this in obj.res:

let magic = Primitive_object_extern.magic

This would need a copy of the external instead.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

I think we can merge and investigate.
This is a true reflection of what we have in prod.

@cknitt
Copy link
Member Author

cknitt commented Oct 4, 2024

I'd prefer to keep this open until the obvious regressions with Obj.magic etc. are sorted out.

@cknitt
Copy link
Member Author

cknitt commented Oct 4, 2024

@cometkim I think all primitive_xxx_extern.res need to be removed and their external definitions copied to where they are needed.

@cometkim
Copy link
Member

cometkim commented Oct 5, 2024

I think all primitive_xxx_extern.res need to be removed and their external definitions copied to where they are needed.

The primitive_xxx_extern.res modules are used to avoid cross-references and unintentional recursive declarations in primitive modules.

They may not be referenced anywhere else, except in exceptional cases such as types.


And the only reason I didn't deprecate Obj.magic is because it's heavily used. I don't think it's useful.

@cknitt
Copy link
Member Author

cknitt commented Oct 5, 2024

@cometkim See this commit for fixing the output for Obj.magic: e172eb5

What's important is that user code directly uses the externals.

Shall I try to fix all such issues or do you want to do it?

@cometkim
Copy link
Member

cometkim commented Oct 5, 2024

That fix is what I was expected; As Obj is not a compiler primitive, it should not directly access to xxx_extern modules. What other changes do you expect?

@cknitt
Copy link
Member Author

cknitt commented Oct 6, 2024

You are right that we do not need to remove the xxx_extern modules. It is sufficient to get rid of usage like

let magic = Primitive_object_extern.magic

So far I did that for Primitive_object_extern only, but I still need to look at similar usages for the other xxx_extern modules.

@cknitt cknitt force-pushed the disable-cross-module-opt branch 2 times, most recently from 092019a to 69146c7 Compare October 7, 2024 05:46
@cknitt cknitt marked this pull request as ready for review October 7, 2024 05:57
@cknitt
Copy link
Member Author

cknitt commented Oct 7, 2024

After the fixes, changes look good to me now.

@cknitt cknitt changed the title Experiment: disable -bs-cross-module-opt for tests Disable -bs-cross-module-opt for tests Oct 7, 2024
@cknitt cknitt requested review from cristianoc and zth October 7, 2024 05:59
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

sweet

@cknitt cknitt merged commit b035602 into rescript-lang:master Oct 7, 2024
20 checks passed
@cknitt cknitt deleted the disable-cross-module-opt branch October 7, 2024 06:20
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