-
Notifications
You must be signed in to change notification settings - Fork 471
compiler: update typecore; tests: add VariantCoercionConstructUsed; d… #7839
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
992088e
7f1f3c4
1844a6c
6012c86
5d5e111
bfcef55
2c6a0e9
7da7c05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
type a = A | B | ||
|
||
// b spreads a and adds one more constructor | ||
type b = | ||
| ...a | ||
| C | ||
|
||
let upcast = (x: a): b => (x :> b) | ||
|
||
let _ = upcast(A) |
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
found a compiler bug: https://github.com/rescript-lang/rescript/issues/7838 (due to the coercing between variants) | ||
GitHub | ||
Incorrect compiler warning (constructor is never used to build valu... | ||
https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=LYewJgrgNgpgBAUQG4wHYBc4F44G8BQcc6AngA7yY4A+iKGAgvoXLJgBYCGqYs2cAChj10ALmIBKbAD48LIgGcA7gEt0AY3ZxhaTASJFayXQxmCJ8uAF8WNm-l... | ||
Incorrect compiler warning (constructor is never used to build valu... | ||
[09:34]Gabriel Nordeborn: What is wrong here...? | ||
[09:35]jaap: The warning? | ||
[09:36]jaap: The variants are constructed in handle | ||
[09:37]jaap: I mean, the coercion constructs the variants | ||
[09:38]jaap: so if we have variantA :> variantB, we can mark that variantB is always constructed | ||
[09:38]Gabriel Nordeborn: Right, so if you adapt the example to not use coercion, it marks them as constructed? | ||
[09:39]Gabriel Nordeborn: It's good if you write this in the PR issue | ||
[09:39]Gabriel Nordeborn: Right now, just looking at the example, I did not understand what you're referring to | ||
[09:41]jaap: Yes | ||
[09:41]jaap: https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=LYewJgrgNgpgBAUQG4wHYBc4F44G8BQcc6AngA7yY4A+iKGAgvoXLJgBYCGqYs2cAChj10ALmIBKbAD48LIgGcA7gEt0AY3ZxhaTASJFayXQxmCJ8uAF8WNm-lCQ+AZRgAnFG+MYA4iHDi+qwwmGgAjhAwkeLe6AB0VLIQqGr4VvxBpBTE2Ja0cQWxCcxEbHBcPHw4nAokqOqCOhji6FJYskGKqhpaTXp5dCZmAhYG1rYsZeGRkfxCIjEiCW2yFbwwAspqmtoicoaDjGaxTERWEgC00ioA5qggbjBpzI7Q8K4e7rEAQpxggZMQtpUBEojBFroEmZkql0jhMuRKLkxvlCkt0CVghxuOt+DU6g15roWit9gYtj1droyQcTsNRgYbGdAaEQTN4Dgic1DvFWmY1rAuZhRLJWldbvdHs8gA | ||
ReScript Documentation | ||
ReScript Playground | ||
Try ReScript in the browser | ||
ReScript Playground | ||
[09:41]jaap: here with two examples | ||
[09:41]jaap: also updated the ticket | ||
[09:44]cristianoc: The type system was created with no coercion in mind. | ||
[09:45]cristianoc: So fix warning —-> compiler error? | ||
[09:47]Gabriel Nordeborn: @jaap is this not a better example? https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=LYewJgrgNgpgBAUQG4wHYBc4F44G8BQcc6AngA7yY7JroCC+hcsmAFgIapizZwAUMFBgBcxAJTYAfHiZEAzgHcAlugDGrOINoyiRAD6Ih9KfzGy4AXyZWr+UJB4BlGACcULmhgDiIcKIJEpBTEvAYAdBGe6GHojEQsmqgAjhAwqaJU0hCoKvgWvAHE5JShcBFhUTFxzDBsnNzwOOxyJKiq-Foi4iaF8spqGp2YvXAGUXQmfGa6ltZMCWgpaY0dRhkSWNIcXLACRmIAtJJKAOaoIC4weYz20PDObq5RAELsYP7ztYlL6Ya0MSZsrl8jhCkESjhwpEjFVPnUditmq12ntaOseuZFCp1JojDpdGMjBNNqZzFYiOSaphFqlUrxUV1KugNlt6rsGZhhNJmYdjmcLlcrEA | ||
ReScript Documentation | ||
ReScript Playground | ||
Try ReScript in the browser | ||
ReScript Playground | ||
[09:47]Gabriel Nordeborn: Using the switch fixes the warning of course because you're now constructing it explicitly | ||
[09:47]Gabriel Nordeborn: If you were to add the switch instead of coercion that'd work as well | ||
[09:48]Gabriel Nordeborn: But from what I understand the problem is that there's a value of a type that gets coerced to another type, and that other type is not marked as used | ||
[09:49]Gabriel Nordeborn: The value that's not coerced is marked, but the new type from the coercion is not marked | ||
[10:35]jaap: @cristianoc there is no way to fix this warning - you need to convert it to a switch expression, but that is a lot of redudant code potentially | ||
[10:36]cristianoc: That’s actually better. So the message is at least not inconsistent. | ||
[10:36]jaap: but the tricky part is that when coercing, the members that are in variantA need to marked as used, but not the new ones | ||
[10:36]cristianoc: I was worried: you remove the case, then get a compile error | ||
[10:37]jaap: Yes you do get an error - so it's not swallowing that | ||
[10:37]jaap: if you include it (warning) if you remove it (error) | ||
[10:38]cristianoc: There’s no such thing in the compiler. As ocaml does not have that. So it will be a genuine extension to handle this. | ||
And I don’t even know if possible. | ||
Is this global property? If so the. Compiler can’t do it. | ||
[10:38]jaap: but yeah seems to be a gap in the type system after adding the nice spreading of variants and coercing | ||
[10:39]jaap: No it's local - in the example you see the module signature added, so it only happens if you make the type private, and then coerce it | ||
[10:40]jaap: I guess there is some mechanism to mark members as constructed - and coercion is not doing that | ||
[10:40]cristianoc: Ok this is enough discussion. | ||
Paste it in a prompt and see if it gets close. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
|
||
[1;33mWarning number 37[0m | ||
[36m/.../fixtures/VariantCoercionWarnOnC.res[0m:[2m9:3-11:7[0m | ||
|
||
7 [2m│[0m let log: a => unit | ||
8 [2m│[0m } = { | ||
[1;33m9[0m [2m│[0m [1;33mtype b =[0m | ||
[1;33m10[0m [2m│[0m [1;33m | ...a[0m | ||
[1;33m11[0m [2m│[0m [1;33m | C[0m | ||
12 [2m│[0m | ||
13 [2m│[0m let log = (x: a) => Js.log((x :> b)) | ||
|
||
unused constructor C. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// Ensure unused-constructor warning still fires for new members introduced | ||
// only on the target side of a coercion. | ||
|
||
type a = A | B | ||
|
||
module M: { | ||
let log: a => unit | ||
} = { | ||
type b = | ||
| ...a | ||
| C | ||
|
||
let log = (x: a) => Js.log((x :> b)) | ||
} | ||
|
||
let _ = M.log(A) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Generated by ReScript, PLEASE EDIT WITH CARE | ||
|
||
|
||
function log(x) { | ||
console.log(x); | ||
} | ||
|
||
let DoNotWarn = { | ||
log: log | ||
}; | ||
|
||
console.log("A"); | ||
|
||
export { | ||
DoNotWarn, | ||
} | ||
/* Not a pure module */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// Repro for incorrect "constructor ... is never used" warning with coercions | ||
// This should compile cleanly without warnings when coercing from a -> b. | ||
|
||
type a = A | B | ||
module DoNotWarn: { | ||
let log: a => unit | ||
} = { | ||
type b = | ||
| ...a | ||
| C | ||
|
||
let log = (x: a) => Js.log((x :> b)) | ||
} | ||
|
||
let _ = DoNotWarn.log(A) |
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.
@cristianoc what does this mean in practice? I don't get it.
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.
not sure
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.
@codex what's the answer here?
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.
The code around line 3064 handles cases where the type path used in the code (
p_tgt
) differs from the path of its concrete declaration (p_tgt_conc
). This occurs when a module signature exposes a variant type via a private or abstract alias. When such a coercion happens, the code marks the overlapping constructors as “used” on both the concrete declaration and the exposed alias so that “constructor … is never used” warnings are cleared for both locations compiler/ml/typecore.mlL3051-L3064View task →
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.
@codex explain more in detail the terms used in your response and the mechanism to mark a constructor never used.
Don't assume familiarity with the representation of types and type environments, but introduce enough context to understand this change.
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.
Summary
p_tgt
(path used in source code) versusp_tgt_conc
(resolved declaration path) and to explain howEnv.mark_constructor_used
records constructor usage on both the concrete type and any exposed alias so “constructor … is never used” warnings are cleared compiler/ml/typecore.mlL3051-L3064Testing
make format
(fails: dune: command not found)make test
(fails: KeyboardInterrupt during ninja build)Notes
apt-get update
failed with 403 errors, so required dependencies (e.g.,dune
) could not be installedView task →
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.
If we remove this special case code, does the fix still work? If so I think we should remove it, and then add it again in the future if we discover real issues it would fix.