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

fix: Make def collector ordering more deterministic #2515

Merged
merged 5 commits into from
Sep 5, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Aug 31, 2023

Description

Problem*

Resolves #2506 partially. The error in the issue is solved since we now always issue an error. The issue before was related to #1122 in which there's no clear ordering we should resolve globals in, so if we reorder the program we can get a different error, but it is at least always deterministic and the program never passes when it should not.

Summary*

To remove as much nondeterminism as possible, I replaced almost all of the hash maps we were iterating over with btree maps. This means in cases where aliases are defined in order the program will always work, but aliases defined backwards or out of order will now always fail to find the type the alias refers to. We'll need a more sophisticated ordering of dependency resolution to fix this.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher jfecher changed the title Make def collector ordering more deterministic fix: Make def collector ordering more deterministic Aug 31, 2023
@alexvitkov
Copy link
Contributor

alexvitkov commented Sep 1, 2023

We'll need a more sophisticated ordering of dependency resolution to fix this.

I'm worried that dependency ordering will not work due to circular dependencies:

struct LinkedList { a: &ref LinkedListAlias }; 
type LinkedListAlias = LinkedList;

I believe this is "semantically valid", although it's pretty dificult to construct a member of A (you have to use std::unsafe::zeroed to create the first instance, and then you can reference it when constructing additional ones). This example is a bit of a stretch, but I'm worried that something like this can arise in a larger program with longer dependency chains.

I believe in the most general case you'd need TypeVariables for the type aliases. I was looking at TypeVariables anyways, since I believe they may be needed to fix #2502, but I don't have a working solution yet.

#2502 is a related problem, but there the conflict is between TypeAliases and Structs, not between the individual TypeAliases - right now if we resolve structs first, they can't contain type aliases, and if we resolve type aliases first, they can't reference structs.

@jfecher
Copy link
Contributor Author

jfecher commented Sep 1, 2023

@alexvitkov in a hypothetical solution with full dependency ordering, any cyclical dependencies we find, we'd report an error for. We'd also have to consider all globally visible items as you mention, so there will no longer be a set "aliases, then structs, then functions" order. It'll depend on where each item is in the dependency tree.

It will also still be possible to create cyclical structures using type variables but you won't be able to actually construct such types.

@jfecher
Copy link
Contributor Author

jfecher commented Sep 1, 2023

I removed the commit adding the fix for #2502, and put up a separate PR instead: #2528

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

Just need those merge conflicts resolved and this looks good to me

@jfecher jfecher added this pull request to the merge queue Sep 5, 2023
Merged via the queue into master with commit d49e0af Sep 5, 2023
13 of 14 checks passed
@jfecher jfecher deleted the jf/definition-ordering branch September 5, 2023 17:41
TomAFrench added a commit that referenced this pull request Sep 5, 2023
* master:
  fix(aztec): fix compilation of `aztec_library.rs` (#2567)
  feat(ssa): Replace values which have previously been constrained with simplified value (#2483)
  fix: Black box func slice handling (#2562)
  chore: Temporarily disable `noir_wasm` test (#2566)
  fix: Make def collector ordering more deterministic (#2515)
  chore: refactor `constant_folding` pass (#2533)
  chore: Cleanup mem2reg pass (#2531)
  chore: Replace hashers of hashmaps in dfg with fxhashes (#2490)
  chore: remove duplicate span from FunctionReturnType (#2546)
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.

Type aliases are not always respected
3 participants