Skip to content

local implies yielding #3532

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

Merged
merged 20 commits into from
Apr 1, 2025
Merged

Conversation

dkalinichenko-js
Copy link
Contributor

Make yielding mode the default if local is set. The change is only limited to syntax parsing and does not change the inference code. To preserve consistency, the global modality/modifier now also implies unyielding by default, and [@local_opt] also varies along the yielding axis.

Subsequent PRs will add yielding annotations to the Capsule API and to the Effects API.

The printer now displays stack_ allocation return values with @ unyielding, as intended.

This change sometimes causes principal and non-principal type inference to diverge when the old syntax for local_ annotations is used. I'm surprised this compiler option affects syntax parsing, but the issue seems benign.

@dkalinichenko-js dkalinichenko-js force-pushed the dkalinichenko/local-implies-yielding branch from 0a38111 to 00ad634 Compare February 6, 2025 20:53
@mshinwell mshinwell added the modes Work on modes. There's some overlap with the `multicore` label, but not strictly so. label Feb 10, 2025
@dkalinichenko-js dkalinichenko-js force-pushed the dkalinichenko/local-implies-yielding branch from 2f1a8ed to bdec230 Compare March 3, 2025 17:42
@dkalinichenko-js
Copy link
Contributor Author

@riaqn I'd appreciate help debugging the divergence between principal and non-principal typing here, I can't figure out why it happens.

@riaqn
Copy link
Contributor

riaqn commented Mar 5, 2025

The principality issue is because some types (for example, string) is immutable_data and crosses yielding. Let's discuss offline.

@dkalinichenko-js dkalinichenko-js force-pushed the dkalinichenko/local-implies-yielding branch from bdec230 to f061df6 Compare March 27, 2025 15:59
@dkalinichenko-js
Copy link
Contributor Author

I changed legacy zapping behavior of yielding to vary based on areality too. This fixed the differences between principal and non-principal typing, so I'm happy to merge this now.

@dkalinichenko-js
Copy link
Contributor Author

I also changed contents of lazys to always be unyielding in addition to global. Otherwise, existing code that relies on the contents of a local lazy_t being at the legacy mode would break.

@dkalinichenko-js
Copy link
Contributor Author

Fixed the issue you pointed out + print order.

Copy link
Contributor

@riaqn riaqn left a comment

Choose a reason for hiding this comment

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

Could you check again if any diff can be minimized? (since the commits has gone back and forth).
Then I think it's good to go.

@dkalinichenko-js dkalinichenko-js force-pushed the dkalinichenko/local-implies-yielding branch from 65e2665 to 1bc5a60 Compare April 1, 2025 15:56
@dkalinichenko-js dkalinichenko-js merged commit c881b34 into main Apr 1, 2025
26 checks passed
@dkalinichenko-js dkalinichenko-js deleted the dkalinichenko/local-implies-yielding branch April 1, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modes Work on modes. There's some overlap with the `multicore` label, but not strictly so.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants