Skip to content

Use type annotations from arguments in let rec #12315

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

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

stedolan
Copy link
Contributor

When typing let rec expressions, the compiler 'approximates' the type of each binding, allowing it to use information about each binding during typechecking. For instance, in a let rec of functions, it knows the arity of each definition early, allowing it to produce partial application warnings for mutually recursive functions.

Currently, type annotations on function parameters are ignored during this approximation. This delays typechecking errors, often in a confusing way. This patch makes approximation use these type annotations. As well as improving errors, this also makes type-based disambiguation work in more cases, for instance:

module M = struct type t = A | B end

let rec f () = g A
and g (x : M.t) = f ()

With this patch, the above typechecks, because the compiler knows that g accepts M.t before typing the body of f.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

The patch looks correct to me, and I agree that it is a reasonable extension of the current behavior.

(Is the absence of a Changes entry intentional?)

Error: This expression has type int but an expression was expected of type
string
|}]

Copy link
Member

Choose a reason for hiding this comment

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

Could you include a case of optional argument, to make sure that this is handled correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@stedolan
Copy link
Contributor Author

(Is the absence of a Changes entry intentional?)

Nope, just hadn't written it yet.

@stedolan stedolan force-pushed the type-args-in-let-rec branch from e93a382 to d6f3c6c Compare June 20, 2023 16:19
@Octachron
Copy link
Member

I fear that the merge of #12210 broke the new test. Sorry about this collision.

@gasche gasche force-pushed the type-args-in-let-rec branch from d6f3c6c to 155690a Compare June 20, 2023 19:08
@gasche
Copy link
Member

gasche commented Jun 20, 2023

I rebased the PR (by force-pushing to the remote branch) to the new testsuite output.

@stedolan
Copy link
Contributor Author

Thanks for rebasing!

@stedolan stedolan merged commit 5babf9b into ocaml:trunk Jun 21, 2023
stedolan added a commit to stedolan/ocaml that referenced this pull request Jul 25, 2023
@stedolan stedolan mentioned this pull request Jul 25, 2023
stedolan added a commit to stedolan/ocaml that referenced this pull request Jul 25, 2023
gasche added a commit that referenced this pull request Jul 25, 2023
voodoos added a commit to voodoos/ocaml that referenced this pull request Feb 12, 2025
voodoos added a commit to voodoos/ocaml that referenced this pull request Feb 17, 2025
voodoos added a commit to voodoos/ocaml that referenced this pull request Feb 17, 2025
@voodoos voodoos mentioned this pull request Feb 17, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants