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

Various uncurried fixes #810

Merged
merged 11 commits into from
Aug 17, 2023
Merged

Various uncurried fixes #810

merged 11 commits into from
Aug 17, 2023

Conversation

zth
Copy link
Collaborator

@zth zth commented Aug 16, 2023

This runs the tests in uncurried mode (+ latest beta) and fixes whatever issues surfaced.

The diff is hard to read because I can't get the printer to print uncurried without the dots. @cristianoc any idea? Getting that to work would make this much easier to follow.

Also, ignore SignatureHelp because fixes for that is in a separate PR.

Closes #772

@zth zth requested a review from cristianoc August 16, 2023 20:33
@cristianoc
Copy link
Collaborator

Might need to turn on uncurried mode explicitly when the printer is called.

@zth zth force-pushed the uncurried-fixes branch from 4feb0ae to 8bbace5 Compare August 17, 2023 10:53
@zth
Copy link
Collaborator Author

zth commented Aug 17, 2023

@@ -49,10 +49,11 @@ let ident ppf id = pp_print_string ppf (ident_name id)
(* Print a path *)

let ident_pervasives = Ident.create_persistent "Pervasives"
let ident_pervasives_u = Ident.create_persistent "PervasivesU"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be backported to the compiler.

let uncurried =
if !Config.uncurried <> Legacy then not uncurried else uncurried
in
let uncurried = Res_uncurried.getDotted ~uncurried !Config.uncurried in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR:ed to the compiler here: rescript-lang/rescript#6353

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does that fix the issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It fixes the printing issues in this repo, yes. There were dots in lots of places there shouldn't be in uncurried mode before this.

@zth zth changed the title [WIP] Various uncurried fixes Various uncurried fixes Aug 17, 2023
@zth zth marked this pull request as ready for review August 17, 2023 11:56
@zth
Copy link
Collaborator Author

zth commented Aug 17, 2023

@cristianoc this is ready for review. Should we run all tests in uncurried mode primarily moving forward, or should I revert back to curried mode? What do you think?

I personally think it might be too soon to run in uncurried mode and optimize for that as the main case given so few have migrated to it yet, but that we should switch over and test the output then and then.

There's the alternative of trying to set up 2 test suites, one for curried and one for uncurried. But I don't know.

analysis/tests/src/expected/Completion.res.txt Outdated Show resolved Hide resolved
let uncurried =
if !Config.uncurried <> Legacy then not uncurried else uncurried
in
let uncurried = Res_uncurried.getDotted ~uncurried !Config.uncurried in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does that fix the issue?

@cristianoc
Copy link
Collaborator

@cristianoc this is ready for review. Should we run all tests in uncurried mode primarily moving forward, or should I revert back to curried mode? What do you think?

I personally think it might be too soon to run in uncurried mode and optimize for that as the main case given so few have migrated to it yet, but that we should switch over and test the output then and then.

There's the alternative of trying to set up 2 test suites, one for curried and one for uncurried. But I don't know.

Two suites sounds heavy, unless you feel like having that.
Perhaps yes one can test back and forth, and switch to the prevalent one at the time. So no switch yet.

@zth
Copy link
Collaborator Author

zth commented Aug 17, 2023

@cristianoc the test failures are interesting. They're different errors depending on whether it's Linux or Windows, but what they have in common is that "Go To Definition" seems to point at different files depending on platform. This started happening with the later versions of v11 where more things have been ReScriptified.

Any ideas?

@zth
Copy link
Collaborator Author

zth commented Aug 17, 2023

Sorry I pushed another commit that muddied up what I commented on. If you want to have a look at the problem I described, you can check these two failed runs:

@zth zth merged commit 6ded31f into master Aug 17, 2023
@zth zth deleted the uncurried-fixes branch August 17, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create-Interface command in uncurried mode does not preserve @react.component
2 participants