Skip to content

Error message: Suggest module in place of value where it makes sense #7384

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

Merged
merged 3 commits into from
Apr 14, 2025

Conversation

zth
Copy link
Collaborator

@zth zth commented Apr 8, 2025

There are some subtle differences between how certain common things are called in ReScript vs in JS. One example is logging to the console:

// JS
console.log("Hello")

// ReScript
Console.log("Hello")

This difference (value vs module) can be subtle and not easily discoverable. So, this PR does a small addition to the "missing value" error message, and suggests the module version of the identifier if it exists in scope. This is intended to give a small hint for what you might be looking for, instead of just telling you to "go fish".

@zth zth requested review from cknitt, nojaf and cristianoc April 8, 2025 12:12
match as_module with
| None -> ()
| Some module_path ->
Format.fprintf ppf "@,@[<v 2>@,@[%s to use the module @{<info>%a@}?@]@]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "@,@[<v 2>@,@[%s here exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OCaml format printing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I meant more specially the @,@[< incantation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn, just asked an LLM, this is some gourmet camel you are cooking.

a lowercase identifier in JS but a module in ReScript.

'Console' is a typical example, where JS is `console.log` and ReScript is `Console.log`. *)
(* TODO(codemods) Add codemod for refering to the module instead. *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the todo for a code action here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is one of those places we can add a code action for when we get that setup.

@zth zth merged commit 1795684 into master Apr 14, 2025
20 checks passed
const fixtures = readdirSync("fixtures").filter(
fileName => path.extname(fileName) === ".res",
const fixtures = readdirSync(path.join(import.meta.dirname, "fixtures")).filter(
(fileName) => path.extname(fileName) === ".res"
Copy link
Member

Choose a reason for hiding this comment

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

Why did the formatting change here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean the parenthesis on (fileName)?

@zth zth deleted the error-message-did-you-mean-module branch April 14, 2025 11:57
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.

3 participants