Skip to content

improve the error message when require and provide transformers are used outside of require and provide #5044

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rfindler
Copy link
Member

This error message improvement is in response this post on discourse.

@mflatt
Copy link
Member

mflatt commented Jul 19, 2024

This looks ok to me.

I always find "must be used in X" messages tricky, because other macros might expand to X. Maybe "a require form" and "a provide form" here (where "require" and "provide" are still not code-quoted, the same as here).

Although something could rely on distinguishing values that it controls with provide-transformer? vs. procedure?, and this change would break that, it seems unlikely.

@sorawee
Copy link
Collaborator

sorawee commented Jul 19, 2024

"is a provide transformer, and cannot be used in this context"?

Off-topic, but do we really want to keep calling these require/provide transformers? Other forms would call them "expanders".

@mfelleisen
Copy link
Collaborator

"is a provide transformer, and cannot be used in this context"?

Don't use implementations names for error messages. (I know that Americans call it a "cell phone" to honor the implementation technique but it is a mobile phone as opposed to a "wall phone" (which is what kids call the old things). But I don't think that' good practice.)

Perhaps something like this

"must be used to specify a contract on an import [from another module] and not by itself"

and we could omit the bracketed words.

@rfindler
Copy link
Member Author

This change isn't just about contracts, it turns out that many of the things that go inside a require have bad error messages, for example:

#lang racket
(rename-in racket/list [x y])

produces

rename-in: illegal use of syntax
  value at phase 1: #<rt> in: (rename-in racket/list (x y))

and similarly for prefix-in, combine-in, etc.

I see that the documentation uses the term require-spec; maybe we could use use that in the error message? Perhaps "x: is a require-spec and cannot appear here" or "x: a require-spec must appear inside require or a similar form"?

As for:

Although something could rely on distinguishing values that it controls with provide-transformer? vs. procedure?, and this change would break that, it seems unlikely.

Given that the only way to signal these errors is by making the value be a procedure, it seems like that'd be an unfortunate form of backward compatibility to have to honor. But I agree with you that it is unlikely.

@rfindler
Copy link
Member Author

Or how about:

x: illegal placement for `require-spec`;
 valid positions include the argument to `require`

@rfindler rfindler force-pushed the require-provide-transformer-error-messages branch from 09b7434 to c334ee8 Compare July 23, 2024 14:15
@rfindler
Copy link
Member Author

With the push I just did, this program:

#lang racket
(rename-in racket/list [x y])

(in the file tmp2.rkt) produces this message:

/Users/robby/tmp2.rkt:2:0: rename-in: illegal placement for `require-spec`;
 valid positions include the argument to `require`
  in: (rename-in racket/list (x y))
  location...:
   /Users/robby/tmp2.rkt:2:0
  context...:
   /Users/robby/git/plt/racket/collects/syntax/wrap-modbeg.rkt:46:4

but I'm not sure I've got the quoting conventions right. I have a memory that the quotes should be `' but I don't see that in the error message conventions documentation. What's the right thing?

@mfelleisen
Copy link
Collaborator

I think this is an improvement.

(I thought Matthew's policy for regular racket was to not make suggestions, such as "valid positions include the argument to require". My personal preference is to not refer to subforms such as the inside of require as "argument".)

@rfindler
Copy link
Member Author

I think this is an improvement.

(I thought Matthew's policy for regular racket was to not make suggestions, such as "valid positions include the argument to require".

Ah, right, I remember something like that. Unfortunately, searching in the docs for "require-spec" doesn't turn up anything useful. But perhaps this is a separate problem, where the non-terminals in blueboxes need better support in the documentation (I often miss the ability to from a reference of one of those to its definition.)

My personal preference is to not refer to subforms such as the inside of require as "argument".)

Do you mind offering an edit to the error message to fix that? (I can put it into the PR and I'll delete the part after the semicolon at the same time.)

@mfelleisen
Copy link
Collaborator

mfelleisen commented Jul 23, 2024 via email

@mflatt
Copy link
Member

mflatt commented Jul 23, 2024

Some opinions on:

x: illegal placement for `require-spec`;
 valid positions include the argument to `require`
  • I don't think require-spec should be in backquotes, since it's not a syntatic form name. Also, as you say, it probably doesn't count as a technical term, just a nonterminal in some grammars.
  • I agree with @mfelleisen about avoiding the word "argument" for a syntactic form, as opposed to a function call / procedure application.
  • I've become less resistant to suggesting repairs in error messages — usually with something like "possible solution" to clarify that it's a guess that may not apply to the current context, but that seems like a lot here. The drawback to mentioning something like require is that the current context might not have that binding. Overall, though, the benefit of being clearer for most (nearly all?) contexts may outweigh the drawback here.

Here's another concrete suggestion to refine the message:

x: illegal use of an import form;
 valid positions include immediately within `require`

@sorawee: A require or provide transformer is not an "expander" in the same sense as many other contexts, because its input and output are not in the same kind of thing. That's why I went with "transformer" originally. Granted, "provide pre-transformer" is pretty awkward terminology for something that does turn out to be an expander, but it would be even more confusing to have "provide expander" and "provide transformer" as different things. That's the rationale that got us here, anyway.

Quoting conventions: use backquotes instead of backquote-forwardquote. There may be lingering backquote-forwardquotes, but I've been trying to convert them all.

@shhyou shhyou added the macro system Expander, Syntax Parse, other #lang racket macros label Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macro system Expander, Syntax Parse, other #lang racket macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants