-
Notifications
You must be signed in to change notification settings - Fork 57
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
Bugfixes for type resolutions #892
Conversation
(typeArgs |> List.map Shared.typeToString |> String.concat ", ") | ||
(typeParams |> List.map Shared.typeToString |> String.concat ", ") | ||
|
||
let rec pathFromTypeExpr (t : Types.type_expr) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved
Some path | ||
| _ -> None | ||
|
||
let printRecordFromFields ?name (fields : field list) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved
|> String.concat ", ") | ||
^ "}" | ||
|
||
let rec extractedTypeToString ?(inner = false) = function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved
@@ -48,6 +112,59 @@ let instantiateType ~typeParams ~typeArgs (t : Types.type_expr) = | |||
in | |||
loop t | |||
|
|||
let instantiateType2 ?(typeArgContext : typeArgContext option) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly different to the existing instantiateType
- this instantiates via comparing Tvar
name only, while the existing one structurally checks whether the Tvar
types are the same. In this new solution they won't be the exact same object since we're moving across type manifests and what not.
(debugLogTypeArgContext typeArgContext)); | ||
typeArgContext | ||
|
||
let rec extractFunctionType2 ?typeArgContext ~env ~package typ = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as existing one but doesn't instantiate. Could potentially remove this one all together in the larger refactor as it's only special because it can do lookup of types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive set of changes.
| [], _ | _, [] -> t | ||
in | ||
|
||
let rec loop (t : Types.type_expr) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any concerns with termination here, or are the cases clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the cases are clear. Or at least this has been around forever and worked so far AFAICT (the code is just copied).
8febff9
to
4c4c531
Compare
This clears up a number of issues for resolving types across/within files, moving through type manifests, and what not. It'll make pattern completion and expression completion a lot more stable.
This introduces a fair bit of duplication and so on. There's a lot of technical debt now with duplicated functions and what not. I plan on taking a larger grip at fixing that and improving the code in general after this has landed. Just need to find a way to do it incrementally.
However, I want to land this before cleaning things up, because this improves the completion situation a lot as is, and cleaning up won't give any new benefits to users more than us having more maintainable code. So, what I'm saying is bear with me with the extra things added here and it'll be cleaned up soon.