You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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.
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.
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.
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
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
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.
The text was updated successfully, but these errors were encountered: