chore: Stop cloning FuncMeta
#3968
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
Will look at this on Monday. |
TomAFrench
added a commit
that referenced
this pull request
Jan 8, 2024
* master: chore: Stop cloning `FuncMeta` (#3968) feat: add MVP `nargo export` command (#3870) chore: clippy and general cleanup of debugger (#3957) chore: updating the noirjs tutorial to match stable + some other improvements (#3929) chore: add `insert_range_check` method to `FunctionBuilder` (#3959) chore: remove usage of term "preprocessed" in favour of "artifact" (#3939)
TomAFrench
added a commit
that referenced
this pull request
Jan 8, 2024
* master: chore: Stop cloning `FuncMeta` (#3968)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 8, 2024
…to generic impls (#3967) # Description ## Problem\* Resolves #3964 ## Summary\* The core problem in #3964 stems from a trait method's generics being different from the corresponding impl's generics. So when the former was instantiated, and later tried to apply to the latter during monomorphization after an impl was selected, the latter would be unchanged. This was fixed by remembering the `TraitMethodId` during monomorphization and calling `try_unify` between the trait method's type and the impl method's type before monomorphizing the impl function. This gives type bindings to translate between the two. ## Additional Context - In creating the above type bindings, we need to bind to the original type variables on the trait without instantiating them. This meant any further impl searches would see that trait as having its monomorphized type, rather than its generic type. This would break impl search since that trait/impl would no longer apply to all the types it used to. I've fixed this by allowing `Type::substitute` to substitute to already-bound type variables so that when the impl search later occurs and the type is instantiated, the instantiation automatically sheds the bindings after its (currently bound) type variables are swapped to fresh type variables. Allowing substitution on already bound type variables is a bit concerning though. It's unclear if this could cause unsoundness later on. ~~I'll open an issue to limit the scope of this by only allowing it during `instantiate` calls instead of all `substitute` calls.~~ I've just implemented the change directly in this PR instead. I split `substitute` into `substitute` and `force_substitute`. - There's also a mostly unrelated change in this PR. `function_meta` now returns a reference instead of cloning the entire `FunctionMeta` which was quite wasteful. I'll work on breaking this out into a separate PR. (Edit: #3968) ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves
Summary*
This PR frees us from the tyranny of cloning
FuncMetaevery time we want to access them viaNodeInterner::func_meta. Now we will be graced with a very slight, likely imperceptible performance increase.Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.