Some type inference cleanups #227
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
apoelstra
force-pushed
the
2024-06--types-1
branch
from
7d05d67 to
dd3e6aa
Compare
The arrow module has a ton of `for_*` methods which duplicate the methods in the Constructible traits. Most of these are purely redundant and can be deleted. A couple provide some code reuse, and are demoted to be non-`pub`. Smoe variable names changed in the redundant code, so this will likely not appear as a code-move in the diff, even though it is one. You can just skim this commit.
Refactor only.
This eliminates an `unwrap`, cleans up a bit of code in named_node.rs, and moves every instance of `Type::from` into types/mod.rs, where it will be easy to modify or remove in a later commit. Also removes From<Arc<Final>> for Type, a somewhat-weird From impl which was introduced in #218. It is now superceded by Type::complete. In general I would like to remove From impls from Type because they have an inflexible API and because they have somewhat nonobvious behavior.
The type_name conversion methods use a private TypeConstructible trait which winds up taking more code to use than it saves by reducing reuse. (Though arguably the reuse makes the code more obviously internally consistent.) In future we won't be able to use this trait anyway because the Type constructor will need a type-inference context while the other constructors will not. So delete it now.
apoelstra
force-pushed
the
2024-06--types-1
branch
from
dd3e6aa to
dc83206
Compare
uncomputable
reviewed
Jul 1, 2024
| @@ -227,12 +226,6 @@ impl Final { | |||
| } | |||
| } | |||
|
|
|||
| impl From<Arc<Final>> for Type { | |||
There was a problem hiding this comment.
12a1885: Out of curiosity, what is the nonobvious behavior of From that you referenced in the commit message?
There was a problem hiding this comment.
Actually this one is pretty straightforward -- it is the From impls from Bound to Type that are weird. It takes a bound, creates a new union-bound object (which users do not see at all, but which have crazy nonlocal behavior) and constructs a type which initially has that bound and no others.
In a later PR I may actually make the Bound type private and not user-visible. It's not actually used outside of the types module (including types/arrow.rs which actually implements the type inference rules) and it's not obvious what it's suppose to mean for users.
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.
Over the weekend I largely rewrote the type inference API to allocate bounds using a slab allocator so that we don't leak memory when we construct reference cycles. These changes seemed independently useful and could be PR'd on their own.