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

Add simpler entry points to const eval for common usages. #66877

Merged
merged 1 commit into from
Dec 22, 2019

Conversation

BenLewis-Seequent
Copy link

I found the tcx.const_eval API to be complex/awkward to work with, because of the inherent complexity from all of the different situations it is called from. Though it mainly used in one of the following ways:

  • Evaluates the value of a constant without any substitutions, e.g. evaluating a static, discriminant, etc.
  • Evaluates the value of a resolved instance of a constant. this happens when evaluating unevaluated constants or normalising trait constants.
  • Evaluates a promoted constant.

This PR adds three new functions const_eval_mono, const_eval_resolve, and const_eval_promoted to TyCtxt, which each cater to one of the three ways tcx.const_eval
is normally used.

@matthewjasper
Copy link
Contributor

r? @oli-obk
cc @eddyb

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-29T20:16:09.2724085Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-29T20:16:09.2887227Z ##[command]git config gc.auto 0
2019-11-29T20:16:09.2952066Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-29T20:16:10.1083580Z ##[command]git config --get-all http.proxy
2019-11-29T20:16:10.1094395Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66877/merge:refs/remotes/pull/66877/merge
---
2019-11-29T20:21:45.1084103Z    Compiling serde_json v1.0.40
2019-11-29T20:21:46.6978396Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-29T20:21:56.9171684Z     Finished release [optimized] target(s) in 1m 21s
2019-11-29T20:21:56.9262880Z tidy check
2019-11-29T20:21:57.7391025Z tidy error: /checkout/src/librustc/mir/interpret/mod.rs:611: line longer than 100 chars
2019-11-29T20:21:57.7391459Z tidy error: /checkout/src/librustc/mir/interpret/mod.rs: missing trailing newline
2019-11-29T20:21:59.5172759Z some tidy checks failed
2019-11-29T20:21:59.5178863Z Found 486 error codes
2019-11-29T20:21:59.5178934Z Found 0 error codes with no tests
2019-11-29T20:21:59.5178981Z Done!
2019-11-29T20:21:59.5178981Z Done!
2019-11-29T20:21:59.5184647Z 
2019-11-29T20:21:59.5184980Z 
2019-11-29T20:21:59.5185828Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-29T20:21:59.5186183Z 
2019-11-29T20:21:59.5186280Z 
2019-11-29T20:21:59.5186567Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-29T20:21:59.5186999Z Build completed unsuccessfully in 0:01:24
2019-11-29T20:21:59.5186999Z Build completed unsuccessfully in 0:01:24
2019-11-29T20:21:59.5242662Z == clock drift check ==
2019-11-29T20:21:59.5263961Z   local time: Fri Nov 29 20:21:59 UTC 2019
2019-11-29T20:21:59.8129915Z   network time: Fri, 29 Nov 2019 20:21:59 GMT
2019-11-29T20:21:59.8132404Z == end clock drift check ==
2019-11-29T20:22:01.1054298Z 
2019-11-29T20:22:01.1152256Z ##[error]Bash exited with code '1'.
2019-11-29T20:22:01.1178757Z ##[section]Starting: Checkout
2019-11-29T20:22:01.1180412Z ==============================================================================
2019-11-29T20:22:01.1180667Z Task         : Get sources
2019-11-29T20:22:01.1180723Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@@ -554,3 +556,70 @@ pub fn truncate(value: u128, size: Size) -> u128 {
// Truncate (shift left to drop out leftover values, shift right to fill with zeroes).
(value << shift) >> shift
}


impl<'tcx> TyCtxt<'tcx> {
Copy link
Member

Choose a reason for hiding this comment

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

Why are these added in this file, as opposed to next to const_eval or so?

Scrolling over this file, the only coherent description of it that I can come up with is "random stuff".^^

Copy link
Author

Choose a reason for hiding this comment

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

They need to be within librustc as they are used inside it. I'm not sure where the best place is.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just put them into a submodule of this module. queries could work as a name.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Maybe rename the query to const_eval_global_id (or if it is unused except forin the three wrappers, name it const_eval_internal and leave a comment on it suggesting the use of the wrappers).

I worry that otherwise we'll just reintroduce calls to it that manually do all the boilerplate

src/librustc/mir/interpret/mod.rs Outdated Show resolved Hide resolved
@BenLewis-Seequent
Copy link
Author

I introduced another wrapper const_eval_instance so I could rename the query to const_eval_internal, which is only used by these wrappers and also inside const_eval_internal itself. Although I'm not sure that const_eval_instance is even needed as the only uses of it is for the type_name intrinsic, which could use const_eval_resolve, but we already have the instance.

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2019

So we have const_eval_raw and const_eval_internal? That seems potentially confusing. Please make sure that at least both some with extensive doc comments explaining their difference.

@hdhoang hdhoang added C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 5, 2019
@joelpalmer
Copy link

Ping from Triage: Hi @Skinny121 - any updates?

@BenLewis-Seequent
Copy link
Author

@RalfJung I have added to the doc comments explaining the difference between both of these queries.

@bors
Copy link
Contributor

bors commented Dec 11, 2019

☔ The latest upstream changes (presumably #67202) made this pull request unmergeable. Please resolve the merge conflicts.

src/librustc/query/mod.rs Outdated Show resolved Hide resolved
@varkor varkor added A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` and removed A-const-generics Area: const generics (parameters and arguments) labels Dec 14, 2019
@BenLewis-Seequent
Copy link
Author

@RalfJung @oli-obk Is there anything more I need to change?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 22, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 22, 2019

📌 Commit c010d84 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Dec 22, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 22, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2019
…r=oli-obk

Add simpler entry points to const eval for common usages.

I found the `tcx.const_eval` API to be complex/awkward to work with, because of the inherent complexity from all of the different situations it is called from. Though it mainly used in one of the following ways:
- Evaluates the value of a constant without any substitutions, e.g. evaluating a static, discriminant, etc.
- Evaluates the value of a resolved instance of a constant. this happens when evaluating unevaluated constants or normalising trait constants.
- Evaluates a promoted constant.

This PR adds three new functions `const_eval_mono`, `const_eval_resolve`, and `const_eval_promoted` to `TyCtxt`, which each cater to one of the three ways `tcx.const_eval`
 is normally used.
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2019
…r=oli-obk

Add simpler entry points to const eval for common usages.

I found the `tcx.const_eval` API to be complex/awkward to work with, because of the inherent complexity from all of the different situations it is called from. Though it mainly used in one of the following ways:
- Evaluates the value of a constant without any substitutions, e.g. evaluating a static, discriminant, etc.
- Evaluates the value of a resolved instance of a constant. this happens when evaluating unevaluated constants or normalising trait constants.
- Evaluates a promoted constant.

This PR adds three new functions `const_eval_mono`, `const_eval_resolve`, and `const_eval_promoted` to `TyCtxt`, which each cater to one of the three ways `tcx.const_eval`
 is normally used.
Centril added a commit to Centril/rust that referenced this pull request Dec 22, 2019
…r=oli-obk

Add simpler entry points to const eval for common usages.

I found the `tcx.const_eval` API to be complex/awkward to work with, because of the inherent complexity from all of the different situations it is called from. Though it mainly used in one of the following ways:
- Evaluates the value of a constant without any substitutions, e.g. evaluating a static, discriminant, etc.
- Evaluates the value of a resolved instance of a constant. this happens when evaluating unevaluated constants or normalising trait constants.
- Evaluates a promoted constant.

This PR adds three new functions `const_eval_mono`, `const_eval_resolve`, and `const_eval_promoted` to `TyCtxt`, which each cater to one of the three ways `tcx.const_eval`
 is normally used.
bors added a commit that referenced this pull request Dec 22, 2019
Rollup of 8 pull requests

Successful merges:

 - #66877 (Add simpler entry points to const eval for common usages.)
 - #67299 (Add `ImmTy::try_from_(u)int` methods)
 - #67487 (Rustdoc mutability removal)
 - #67499 (Misc MIR building cleanups)
 - #67506 (Remove iter_private.rs)
 - #67508 (Fix typo in path parser name)
 - #67519 (Document why Any is not an unsafe trait)
 - #67525 (Utilize rust-lang/rust commit hashes in toolstate)

Failed merges:

r? @ghost
@bors bors merged commit c010d84 into rust-lang:master Dec 22, 2019
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Dec 22, 2019
JohnTitor added a commit to JohnTitor/rust-clippy that referenced this pull request Dec 23, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. F-const_generics `#![feature(const_generics)]` 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.

None yet