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

Analysis tests fixes #7295

Merged
merged 8 commits into from
Feb 15, 2025
Merged

Analysis tests fixes #7295

merged 8 commits into from
Feb 15, 2025

Conversation

zth
Copy link
Collaborator

@zth zth commented Feb 15, 2025

  • Removes the builtInModules config concept, since Core is now in the compiler repo and Js/Belt is never to be preferred
  • Moves all tests in analysis to use Core
  • Removes/cleans up a few things/checks related to handling multiple versions of the ReScript compiler - the current editor tooling only ever deals with exactly the version of the compiler it's included in

(* regexps *)
create "%re()" ~insertText:"%re(\"/$0/g\")" ~includesSnippets:true
create "/<regexp>/g" ~insertText:"/$0/g" ~includesSnippets:true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can do this now since regexp syntax is now 1st class.

Comment on lines -151 to -205
builtInCompletionModules =
(if
opens_from_bsc_flags
|> List.find_opt (fun opn ->
match opn with
| ["RescriptCore"] -> true
| _ -> false)
|> Option.is_some
|| fst rescriptVersion >= 12
then
{
arrayModulePath = ["Array"];
optionModulePath = ["Option"];
stringModulePath = ["String"];
intModulePath = ["Int"];
floatModulePath = ["Float"];
promiseModulePath = ["Promise"];
listModulePath = ["List"];
resultModulePath = ["Result"];
exnModulePath = ["Exn"];
regexpModulePath = ["RegExp"];
}
else if
opens_from_bsc_flags
|> List.find_opt (fun opn ->
match opn with
| ["Belt"] -> true
| _ -> false)
|> Option.is_some
then
{
arrayModulePath = ["Array"];
optionModulePath = ["Option"];
stringModulePath = ["Js"; "String2"];
intModulePath = ["Int"];
floatModulePath = ["Float"];
promiseModulePath = ["Js"; "Promise"];
listModulePath = ["List"];
resultModulePath = ["Result"];
exnModulePath = ["Js"; "Exn"];
regexpModulePath = ["Js"; "Re"];
}
else
{
arrayModulePath = ["Js"; "Array2"];
optionModulePath = ["Belt"; "Option"];
stringModulePath = ["Js"; "String2"];
intModulePath = ["Belt"; "Int"];
floatModulePath = ["Belt"; "Float"];
promiseModulePath = ["Js"; "Promise"];
listModulePath = ["Belt"; "List"];
resultModulePath = ["Belt"; "Result"];
exnModulePath = ["Js"; "Exn"];
regexpModulePath = ["Js"; "Re"];
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

builtInCompletionModules are not needed now that Core is in the compiler repo. But, we are going to make these configurable through the project config soon.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ 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: 868774f Previous: 5d9c682 Ratio
Parse RedBlackTree.res - time/run 1.8970486066666665 ms 1.3523686533333334 ms 1.40
Print RedBlackTree.res - time/run 3.305231513333333 ms 1.98472278 ms 1.67
Print RedBlackTreeNoComments.res - time/run 3.0024389466666666 ms 1.8453905533333332 ms 1.63
Parse Napkinscript.res - time/run 65.50723582666667 ms 42.23872597333333 ms 1.55
Print Napkinscript.res - time/run 113.43309909999999 ms 60.21953604666667 ms 1.88
Parse HeroGraphic.res - time/run 8.551319106666666 ms 5.70670238 ms 1.50
Print HeroGraphic.res - time/run 14.501224480000001 ms 8.188623 ms 1.77

This comment was automatically generated by workflow using github-action-benchmark.

Comment on lines +689 to +690
XXX Not found!
[]
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 change is fine for now. It does highlight a problem, but it's something we'll fix separately.

ContextPath Value[Int, toString](Nolabel)
ContextPath Value[Int, toString]
Path Int.toString
[]
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 highlights that we've reduced the scope of the unsaved-completion-inference logic previously for pipes. In practice this does not matter though, because incremental type checking in the editor tooling takes care of this in a much better way.

Will remove these tests in another PR later on, but don't want to touch them for now.

@zth zth requested a review from cknitt February 15, 2025 15:53
@zth zth marked this pull request as ready for review February 15, 2025 15:53
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Nice work! 🎉

@@ -1016,7 +1016,7 @@ Package opens Pervasives.JsxModules.place holder
ContextPath Value[xn]
Path xn
[{
"label": "Js.Exn.Error(error)",
"label": "Exn.t.Error(error)",
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. Fixed!

@zth zth enabled auto-merge (squash) February 15, 2025 17:10
@zth zth merged commit 0f84ac1 into master Feb 15, 2025
20 checks passed
@zth zth deleted the analysis-tests-fixes branch February 16, 2025 08:49
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.

2 participants