Skip to content
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

refactoring: use a structured representation of the first argument of the Typedecl_variance.Bad_variance exception #7924

Closed
vicuna opened this issue Feb 20, 2019 · 4 comments

Comments

@vicuna
Copy link

vicuna commented Feb 20, 2019

Original bug ID: 7924
Reporter: @gasche
Status: acknowledged (set by @gasche on 2019-02-20T15:36:54Z)
Resolution: open
Priority: low
Severity: feature
Category: typing
Tags: junior_job
Monitored by: @nojb

Bug description

typing/typedecl_variance.ml currently contains

type error =
| Bad_variance of int * surface_variance * surface_variance
| Varying_anonymous

but the 'int' argument of Bad_variance is sometimes an integer that represents something real, and sometimes a weird encoding of an error condition represented as -1, -2 or -3.

The code would be nicer to read if this was represented as a proper datatype, something like

type variance_error =
| Variance_not_satisfied of int
| No_variable
| Variance_not_reflected
| Variance_not_deducible

where No_variable corresponds to -2, Variance_not_reflected to -1 and Variance_not_deducible to -3 -- to understand the names, see the error-reporting code in typing/typedecl.ml.

Additional information

It might be possible to preserve location information on the erroring variance in the Variance_not_reflected and Variance_not_deducible cases (that is, have an "of int" parameter like in the Variance_not_satisfied case, and use it in error message). But this requires a finer-grained understanding of the code that checks variances in the type-checker, and is out of scope of this pure refactoring issue.

@karansapolia
Copy link

Hello @gasche I would like to work on this issue. I am fairly new to the ocaml project so I would request some guidance on how to get started on this one.

@gasche
Copy link
Member

gasche commented Mar 17, 2019

Hi @karansapolia, thanks for working on this!

The first step would be to build the current development version ("trunk") of the compiler codebase, following the instructions in HACKING.adoc.

Then you should go to typing/typedecl_variance.ml (the implementation file for the Typedecl_variance module) and typing/typedecl_variance.mli (the interface file), introduce the variance_error type that I proposed above (or a variant that you think is even better), and changing the error type to have

  | Bad_variance of variance_error * surface_variance * surface_variant

This change needs to happen in both the .ml and the .mli file, as these error types are part of the public interface of the module.

Then, what remains to be done is to fix all users of the Bad_variance constructor, in the module and outside, to compile against the new definition. You can do this by just trying to build the compiler codebase, see what fails, fix that, and repeat. (See the message above where I introduce variance_error for the correspondence between its constructors and the int codes -1, -2, -3 etc. currently used in the code.)

If you don't feel confident about the result, feel free to post here a link to the branch in your local fork, for a pre-review or feedback before you send a PR.

@karansapolia
Copy link

Thank you @gasche. That was very helpful. Let me work on it and get back to you 😄

@rmdouglas
Copy link
Contributor

Since it's been a few months since @karansapolia last commented, I thought I'd try my hand at fixing this issue - hope I'm not stepping on any one's toes.

rmdouglas@e5fa268 has the changes that seem to address the issue, switching to using a variant, as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants