Fix issue with gentype and stdlib json. #7378
Conversation
| @@ -253,6 +248,7 @@ let translate_constr ~config ~params_translation ~(path : Path.t) ~type_env = | |||
| | ( (["Js"; "Dict"; "t"] | ["Dict"; "t"] | ["dict"] | ["Stdlib"; "Dict"; "t"]), | |||
| [param_translation] ) -> | |||
| {param_translation with type_ = Dict param_translation.type_} | |||
| | ["Stdlib_JSON"; "t"], [] -> {dependencies = []; type_ = unknown} | |||
There was a problem hiding this comment.
Not sure why here it's ["Stdlib_JSON"; "t"] but e.g. earlier it's ["Stdlib"; "Dict"; "t"].
The aliases seem to be set up in the same way. Any idea @cknitt ?
There was a problem hiding this comment.
Oh it's because of the type annotation: this one:
let jsonEncodeString : JSON.t = JSON.Encode.string("hello")maps to ["Stdlib"; "Dict"; "t"]. So it might be that we need both ways, every time.
There was a problem hiding this comment.
yeah shouldn't we go the safer way just in case?
| | ["Stdlib_JSON"; "t"], [] -> {dependencies = []; type_ = unknown} | |
| | ["Stdlib_JSON"; "t"] | ["Stdlib"; "JSON"; "t"] | ["JSON"; "t"], [] -> {dependencies = []; type_ = unknown} |
There was a problem hiding this comment.
OK I'm now allowing both, but there's still something in the hover:
The two examples show differently in the hover.
And I guess we'd want Stdlib not to appear in either.
Thoughts @zth ?
Also wondering whether there's somewhere else where this shows up, the fact that we have 2 possible paths and only one at most is handled.
There was a problem hiding this comment.
that technically makes sense that the hover name is different because in the first case, JSON.Encode.string returns t, so it shows up with its internal module name while it does with its fully qualified name (Stdlib.JSON.t) when you use JSON.t.
But as you said, it should just be JSON.t in both cases. I guess we can special case stdlib in the extension.
There was a problem hiding this comment.
but this is a bit off-topic for this PR anyway, right?
There was a problem hiding this comment.
but this is a bit off-topic for this PR anyway, right?
Yes unless something is changed deeper down the compiler to make this difference go away everywhere. But I'm not sure about it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.
| Benchmark suite | Current: fecd160 | Previous: 3f06468 | Ratio |
|---|---|---|---|
Print HeroGraphic.res - time/run |
26.265474806666663 ms |
7.840572506666666 ms |
3.35 |
Print HeroGraphic.res - allocs/run |
10571467 words |
1396448 words |
7.57 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
looks good to me now!
Out of curiosity, what was the FB namespace you removed @cristianoc?
Strings had in each character a 4096 bit decryption key. |
| @@ -19,6 +19,7 @@ | |||
| - Fix `Error.fromException`. https://github.com/rescript-lang/rescript/pull/7364 | |||
| - Fix signature of `throw`. https://github.com/rescript-lang/rescript/pull/7365 | |||
| - Fix formatter adds superfluous parens in pipe chain. https://github.com/rescript-lang/rescript/pull/7370 | |||
| - Fix issue wiht gentype and stdlib json. https://github.com/rescript-lang/rescript/pull/7378 | |||
There was a problem hiding this comment.
Needs to go to alpha.12
| - Fix issue wiht gentype and stdlib json. https://github.com/rescript-lang/rescript/pull/7378 | |
| - Fix issue with gentype and stdlib json. https://github.com/rescript-lang/rescript/pull/7378 |
Fixes #7157
Map
JSON.ttounknown.