Refactor TypeResolution to be a simple query based system #1545
Conversation
Deuplicate function elimination can fail when we compile helpers during higher ranked trait bound monomorphization. This because the TyTy::BaseType info can be lost/reset during the compilation process. This adds a second mechanism to match based on the manged names which is a bit more reliable. This patch is required since the query based refactor of the type system so this issue was likely hidden to to using duplicated type info for higher ranked trait bounds.
This patch refactors the type resolution system to introduce a new interface bool query_type (HirId, TyTy::BaseType** result) This is needed in order to properly support forward declared items. Our name resolution system has two parts: 1. Toplevel scan 2. Item resolution The toplevel scan gathers all the nesseacry 'names' into their respective namespace by doing a full toplevel scan and generate canonical paths for each item. The second pass is responsible for drilling down into each structure or function to resolve each field or variable etc. This means our name resolution system supports forward decalred items but our type resolution system did not. This patch removes the toplevel scan from our type resolution pass which is not able to handle all cases such as a function with return type and the type is decalred after the fact or a type alias to a type declared after the fact. The name resolution mappings are resolved so when errors occured here we got errors such as unable to lookup HirId 1234, which meant yes we have 'resolved' this reference to this HirId but we are unable to find any type information for it. This means we needed a new way to figure out the type in a query based way. This is where the new query_type inferface comes in so when we have an HirId we want to resolve the mappings class allows us to figure out what item this is such as: 1. HIR::Item (normal HIR::Function, Struct, TypeAlias, ...) 2. HIR::ImplItem (function, constant, ... within an impl-block) 3. HIR::ImplBlock (Self type on an impl-block) 4. HIR::ExternalItem (extern-block item) The mappings class allows us to simply lookup these HIR nodes and then call the relevant resolver class to compute the type. This patch does not add support for self-referencial types but is the starting point to be able to support such types. Fixes #1455
When resolving local enum's within a Block the name resolution info is not at the top of the stack so this patch introduces a new mappings class for miscellaneous name resolutions which can be used during path analaysis. Fixes #1272
philberty
force-pushed
the
phil/typechecking-refactor
branch
from
90c9bc3 to
b17e698
Compare
| // else if (TREE_CODE (init) == CONSTRUCTOR | ||
| // && !same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (init), | ||
| // type)) | ||
| // { | ||
| // /* See above on initialization of empty bases. */ | ||
| // // gcc_assert (is_empty_class (TREE_TYPE (init)) && !lval); | ||
| // return init; | ||
| // } |
There was a problem hiding this comment.
Dead code? Or will that be useful later on?
There was a problem hiding this comment.
I might want this later on, not 100% sure. Basically we don't really set TYPE_CANONICAL and the qualifiers differ here. So for example:
let a:[i32,5] = {1,2,3,4,5}
We end up with two types in the type system one for the lhs and one for the rhs they are equal (both array types with the same element and capacity). We do have the type hasher which means we avoid needing to use TYPE_CANONICAL because we try really hard to just use the same type as much as possible avoiding duplicates. Otherwise, GCC (I think this might be the fault of the GO abstractions) generate a whole bunch of VIEW_CONVERT_EXPR's to convert between the equivalent structures.
But one has the qualifiers of const and one is not marked const (since the lhs is not mutable) so we end up hitting this !same_type_ignoring_top_level_qualifiers_p and drop into this where yeah this is not an empty class. I am loathed to remove this code only because it makes it easier to debug and see what code we changed in our version of constexpr compared to the C++ one.
We could get around this by avoiding setting the const qualifiers on immutable let bindings but I think its good to give GCC more information for the optimizers to hopefully work well.
| std::map<NodeId, NodeId> resolved_macros; | ||
|
|
||
| // misc | ||
| std::map<NodeId, NodeId> misc_resolved_items; |
There was a problem hiding this comment.
What is considered a misc item?
There was a problem hiding this comment.
This is a bit of a hack, basically in our name resolver we have the different scopes and the original idea I had back last year was well most of the time we can resolve things directly in the name resolver (which turned out not to be the case for any Path items). So when we do resolve things we do:
insert_resolved_{type,name,label,macro} reference_node_id -> definition_node_id
We use these mappings during type checking and compilation to figure things out to NodeId's then use mappings to get over to HirId's to then finally query things.
The issue is for this bug in b17e698 the enum is declared in the block scope and these are handled slightly differently during name resolution.
During name resolution we have the top-level scan then the deep scan. The top-level scan puts all top-level items directly into the uppermost scope rib but for function items we don't drill into these until the 2nd pass during name resolution. This means the declared names are actually existing in nested scopes as you would expect.
We guard this behaviour with calls to append_reference_to_decl in the name resolver which means when you do call inser_resolved_name it looks up in reverse order of the ribs to add a mapping of this node id maps back to this declaration defined in this rib. I though this would be more useful but turns out its just a bit of a pain. The only good part of this assertion is it keeps things honest right now.
This misc_resolved_items is to handle the case of items that are defined within nested scopes. Its a catch all so when the analysis passes lookup resolved node_ids they will always be able to lookup in order of:
name->type->label->misc
So long as no errors have occurred. Using the name resolver information for HIR stuff during lint passes is probably not the best solution but its the only interface we have right now.
|
bors r+ |
|
Build succeeded: |
This patch refactors the type resolution system to introduce a new
interface
bool query_type (HirId, TyTy::BaseType** result)
This is needed in order to properly support forward declared items. Our
name resolution system has two parts:
The toplevel scan gathers all the nesseacry 'names' into their respective
namespace by doing a full toplevel scan and generate canonical paths for
each item. The second pass is responsible for drilling down into each
structure or function to resolve each field or variable etc. This means
our name resolution system supports forward decalred items but our type
resolution system did not.
This patch removes the toplevel scan from our type resolution pass which
is not able to handle all cases such as a function with return type and
the type is decalred after the fact or a type alias to a type declared
after the fact. The name resolution mappings are resolved so when errors
occured here we got errors such as unable to lookup HirId 1234, which meant
yes we have 'resolved' this reference to this HirId but we are unable to
find any type information for it. This means we needed a new way to figure
out the type in a query based way.
This is where the new query_type inferface comes in so when we have an
HirId we want to resolve the mappings class allows us to figure out what
item this is such as:
The mappings class allows us to simply lookup these HIR nodes and then
call the relevant resolver class to compute the type. This patch does not
add support for self-referencial types but is the starting point to be able to support such types.
Fixes #1455
Fixes #1006
Fixes #1073
Fixes #1272