Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bamarsha
Copy link
Contributor

This PR splits out the HM type inference module from #884, as well as additional changes that the module requires. The new module is not used by the compiler, so there's no change to type inference behavior yet (there will be another PR for that).

Additional changes:

  • Added a range field to ResolvedType so that the HM module can generate diagnostics with accurate locations.
  • Split ScopeTools.fs into two files so that the HM module can reference SymbolTracker, and ScopeContext can reference the HM module.
  • Removed WithinIfCondition field from ScopeContext that has been deprecated for 6 months.

Copy link
Contributor

@swernli swernli left a comment

Choose a reason for hiding this comment

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

Changes look good! The InferenceContext.fs file has a lot of logic that is hard to internalize when coming at the code fresh, so I'm wondering if it might be worth having a readme in the TypeInference folder with some brief descriptions of the high level concepts/strategy and a link to external info on Hindley-Milner type inference.

@bamarsha
Copy link
Contributor Author

@swernli I added a readme. Can you take a look at it?

@swernli
Copy link
Contributor

swernli commented Mar 26, 2021

@SamarSha The readme is fantastic! Concise and still very informative for interpreting the accompanying code.

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

I am blocking the PR for now since I believe in its current state it would break some of the IDE commands due to the change in assigning ranges to types even when there is no matching piece of source code.

@bamarsha bamarsha requested a review from bettinaheim March 30, 2021 00:13
@bamarsha bamarsha mentioned this pull request Mar 30, 2021
9 tasks
Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

I think this setup make things a lot cleaner, thanks. I am still waiting for a response regarding SearchAndReplace; we need to update the transformation to not use the deprecated handles. The syntax tree transformation currently walks both the old and the new handle, which I believe would cause issues.

@bettinaheim bettinaheim dismissed their stale review March 30, 2021 04:06

The concern has been partially addressed, and since this goes against a feature branch, making the update of SeearchAndReplace a separate PR makes sense.

@bamarsha bamarsha requested a review from bettinaheim March 30, 2021 21:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants