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

-@: Reject compilation/documentation if none of the supplied --<cfg|rev>s set any compiletest revision unless --forced #27

Open
fmease opened this issue Dec 19, 2024 · 2 comments
Labels
C-medium Technical complexity: Medium K-feature-request Kind: Feature request P-low Priority: Low S-in-progress Status: In progress

Comments

@fmease
Copy link
Owner

fmease commented Dec 19, 2024

It's probably a common user mistake to invoke rruxwry/-@ on a file which defines compiletest revisions without specifying any of them on the command line. Compiletest only tests the file under these exact revisions after all.

However, since rruxwry is meant to be dev-empowering tool, I also want the option to sidestep this hard error (we don't have many yet, mostly just warnings / non-fatal errors). For that I want us to introduce -f/--force.

Steps:

  1. Remove -f/--cargo-feature or at least change the shortflag -f: I don't think it will actually be used much – well, maybe for files minimized from Cargo projects.
  2. If --cargo-feature was removed altogether, consider renaming --rustc-feature, since it no longer needs to be that awkwardly named.
  3. Introduce -f/--force and make use of it, too (e.g., turn some(!) directive parse errors into hard errors unless --force'd).
  4. Remove --rev (I changed my mind: see "discussion" at the end of this bullet point). Just reuse the preexisting --cfg for revisions, too. Adjust the CLI docs for --cfg. Under -@ throw a hard error if none of the --cfgs specify a declared compiletest revision unless --force'd. For example, with //@ revisions: one two rr{c,d} file.rs -@ would throw sth. like error[rruxwry]: no revision specified on the command line // help: specify at least one revision with `--cfg` (+ listing available revisions in some way) (or ~ help: at least one of the supplied --cfgs should mention a revision if the CLI invocation actually specifies --cfgs (+ we can do Levenshtein-distance based typo detection to provide an even better diagnostic)) // ~ use `-f`/`--force` to continue regardless. --cfg . One thing tho: Should we actually require all cfgs to mention legal revisions unless --force'd to run? That might catch more typos for example in //@ revisions: one two rr{c,d} file.rs -@ --cfg one --cfg twp. However, how often would someone actually run multiple revisions at once (note --all-revs (TBI) wouldn't enable all revs but run rruxwry per rev)? We could throw a hard error if the user to specified multiple --cfg if revs were specified unless --force'd. Update: Nah, I guess we should keep --rev (and maybe change it so you're not allowed to specify it multiple times? not sure). If the user wants to specify rev and cfg, they can use --rev one --cfg other. That begs the question: Should --force disable the rev check, too? I guess. So --cfg other and --rev other -f would basically behave the same (diagnostics can mention that).
  5. Slightly unrelated: I think we lack support for "complex cfgs" of the form key="value" (mod shell esc). Add support for that. Well, we do support rr{c,d} f.rs --cfg 'feature = "feature"' for example by virtue of pass-through. However, I'm pretty sure we don't properly support it as a revision (compiletest likely doesn't either). I mean: //@ revisions: '--feature="feature"' '--feature = "feature"' should raise an error bc of the dupe revisions (unless --force'd). We actually need to restrict the grammar of revisions from non-whitespace sequence to IDENT or IDENT="VALUE" (mod shell esc). Not a high prio though.
@fmease fmease added K-feature-request Kind: Feature request C-medium Technical complexity: Medium P-low Priority: Low S-actionable Status: Actionable labels Dec 19, 2024
@fmease fmease changed the title -@: Reject compilation/documentation if none of the supplied --cfgs set a compiletest revision unless --forced -@: Reject compilation/documentation if none of the supplied --<cfg|rev>s set any compiletest revision unless --forced Dec 31, 2024
@fmease fmease added S-in-progress Status: In progress and removed S-actionable Status: Actionable labels Jan 3, 2025
@fmease
Copy link
Owner Author

fmease commented Jan 3, 2025

Mostly implemented in:

  1. cbc6f61
  2. 4668117

The impl doesn't really follow what's written in the issue description (yes I've changed my mind again).

@fmease
Copy link
Owner Author

fmease commented Jan 3, 2025

However, I still want to impl point 5 and maybe --force, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-medium Technical complexity: Medium K-feature-request Kind: Feature request P-low Priority: Low S-in-progress Status: In progress
Projects
None yet
Development

No branches or pull requests

1 participant