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

Unify conditional-const error reporting with non-const error reporting #134732

Merged

Conversation

compiler-errors
Copy link
Member

This PR unifies the error reporting between ConditionallyConstCall and FnCallNonConst so that the former will refer to syntactical sugar like operators by their sugared name, rather than calling all operators "methods". We achieve this by making the "non-const" part of the error message generic over the "non" part so we can plug in "conditionally" instead.

This should ensure that as we constify traits in the standard library, we don't regress error messages for things like ==.

r? fmease or reassign

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 24, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 24, 2024

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors force-pushed the unify-conditional-const-error-reporting branch from 719498a to 541be58 Compare December 24, 2024 19:20
@RalfJung
Copy link
Member

Please give me a chance to review before landing this.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

This looks great :) Just one minor nit.

@compiler-errors compiler-errors force-pushed the unify-conditional-const-error-reporting branch from 541be58 to db51aea Compare January 6, 2025 17:54
@compiler-errors
Copy link
Member Author

Ok this is ready now.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

r=me with comment nit; the other point about whether the error is "this call is wrong" vs "this call is unstable" is pre-existing.

|
= note: calls in constant functions are limited to constant functions, tuple structs and tuple variants
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this, the output seems to be slightly different than for regular calls? Specifically, we don't seem to print this note for things that can be unstably called, we only print it for things that cannot be called at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we don't distinguish cases where:

  1. The feature gate is just not enabled
  2. The feature gate is not enabled, and even if it were the ~const bounds are not satisfied

I think it would be nice to distinguish those, and only have this note be present in (2.), and not (1.). In the (1.) case, it would be even better to leave a note saying something along the lines of "this functionality is not stable in consts yet".

compiler/rustc_const_eval/src/check_consts/ops.rs Outdated Show resolved Hide resolved
|
= note: calls in constant functions are limited to constant functions, tuple structs and tuple variants
= note: see issue #67792 <https://github.com/rust-lang/rust/issues/67792> for more information
= help: add `#![feature(const_trait_impl)]` to the crate attributes to enable
Copy link
Member

Choose a reason for hiding this comment

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

Is this the error that shows up when the function is conditionally-const and we don't have enough trait bounds to know whether it is actually const, or when it is conditionally-const and we do have the trait bounds but we need to enable a nightly feature to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The latter, but we don't distinguish the two per my comment above. I'll probably try to make this less misleading, but I'd prefer to do it as a follow-up. I think we can keep iterating on these errors as need (and as users complain).

@fmease fmease removed their assignment Jan 7, 2025
@compiler-errors compiler-errors force-pushed the unify-conditional-const-error-reporting branch from db51aea to 924000d Compare January 9, 2025 16:20
@compiler-errors
Copy link
Member Author

Edited the comment.

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Jan 9, 2025

📌 Commit 924000d has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2025
…nst-error-reporting, r=RalfJung

Unify conditional-const error reporting with non-const error reporting

This PR unifies the error reporting between `ConditionallyConstCall` and `FnCallNonConst` so that the former will refer to syntactical sugar like operators by their sugared name, rather than calling all operators "methods". We achieve this by making the "non-const" part of the error message generic over the "non" part so we can plug in "conditionally" instead.

This should ensure that as we constify traits in the standard library, we don't regress error messages for things like `==`.

r? fmease or reassign
@bors
Copy link
Contributor

bors commented Jan 9, 2025

⌛ Testing commit 924000d with merge 246ac55...

@bors
Copy link
Contributor

bors commented Jan 9, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 9, 2025
@compiler-errors
Copy link
Member Author

@bors retry

curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
ERROR: failed to download llvm from ci

    HELP: There could be two reasons behind this:
        1) The host triple is not supported for `download-ci-llvm`.
        2) Old builds get deleted after a certain time.
    HELP: In either case, disable `download-ci-llvm` in your config.toml:

    [llvm]
    download-ci-llvm = false

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2025
@matthiaskrgr
Copy link
Member

matthiaskrgr commented Jan 9, 2025

@bors treeclosed=1000

happened here as well #135297 (comment)

@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
ERROR: failed to download llvm from ci

    HELP: There could be two reasons behind this:
        1) The host triple is not supported for `download-ci-llvm`.
        2) Old builds get deleted after a certain time.
    HELP: In either case, disable `download-ci-llvm` in your config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:23

@tgross35
Copy link
Contributor

tgross35 commented Jan 10, 2025

@bors retry

oh, that failure is from before the previous retry so this did nothing

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2025
Rollup of 7 pull requests

Successful merges:

 - rust-lang#132607 (Used pthread name functions returning result for FreeBSD and DragonFly)
 - rust-lang#134693 (proc_macro: Use `ToTokens` trait in `quote` macro)
 - rust-lang#134732 (Unify conditional-const error reporting with non-const error reporting)
 - rust-lang#135083 (Do not ICE when encountering predicates from other items in method error reporting)
 - rust-lang#135251 (Only treat plain literal patterns as short)
 - rust-lang#135320 (Fix typo in `#[coroutine]` gating error)
 - rust-lang#135321 (remove more redundant into() conversions)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9e24b6b into rust-lang:master Jan 10, 2025
6 of 7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 10, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2025
Rollup merge of rust-lang#134732 - compiler-errors:unify-conditional-const-error-reporting, r=RalfJung

Unify conditional-const error reporting with non-const error reporting

This PR unifies the error reporting between `ConditionallyConstCall` and `FnCallNonConst` so that the former will refer to syntactical sugar like operators by their sugared name, rather than calling all operators "methods". We achieve this by making the "non-const" part of the error message generic over the "non" part so we can plug in "conditionally" instead.

This should ensure that as we constify traits in the standard library, we don't regress error messages for things like `==`.

r? fmease or reassign
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants