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

[DRAFT] Make lang items private #72249

Closed
wants to merge 0 commits into from
Closed

[DRAFT] Make lang items private #72249

wants to merge 0 commits into from

Conversation

doctorn
Copy link
Contributor

@doctorn doctorn commented May 15, 2020

I had a go at implementing a new API for dealing with lang_items as discussed in #72240.

This touches a lot of code. 9/10 I feel like it makes the code more readable, or makes it explicit that you should be require()'ing lang items, but there are definitely some exceptions (particularly in some of the librustdoc code). An obvious fix to cut down on the number of require().ok()s would be to add into_option(), but that starts to feel like the old API again.

(This completely breaks clippy and I haven't run the test bench yet.)

r? @oli-obk

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

CI failed pending changes from #72216 (I'll be rebasing on top of it when it lands)

@rust-highfive
Copy link
Collaborator

The job mingw-check 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.
##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 36'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200430.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200430.2/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/a51dadcf-1238-430a-bd07-5dbf3392319a.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72249/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[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/72249/merge:refs/remotes/pull/72249/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
    Checking rustc_feature v0.0.0 (/checkout/src/librustc_feature)
    Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
    Checking rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
    Checking rustc_hir v0.0.0 (/checkout/src/librustc_hir)
    Checking rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
    Checking chalk-rust-ir v0.10.0
    Checking rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
    Checking rustc_parse v0.0.0 (/checkout/src/librustc_parse)
    Checking rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
    Checking rustc_trait_selection v0.0.0 (/checkout/src/librustc_trait_selection)
error[E0599]: no method named `unwrap` found for enum `rustc_hir::lang_items::LangItemRecord` in the current scope
  --> src/librustc_trait_selection/traits/structural_match.rs:72:79
   |
72 |     let structural_peq_def_id = infcx.tcx.lang_items().structural_peq_trait().unwrap();
   |                                                                               ^^^^^^ method not found in `rustc_hir::lang_items::LangItemRecord`
error[E0599]: no method named `unwrap` found for enum `rustc_hir::lang_items::LangItemRecord` in the current scope
  --> src/librustc_trait_selection/traits/structural_match.rs:83:79
   |
   |
83 |     let structural_teq_def_id = infcx.tcx.lang_items().structural_teq_trait().unwrap();
   |                                                                               ^^^^^^ method not found in `rustc_hir::lang_items::LangItemRecord`
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustc_trait_selection`.
error: could not compile `rustc_trait_selection`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:04:03
== clock drift check ==
  local time: Fri May 15 23:49:20 UTC 2020
  local time: Fri May 15 23:49:20 UTC 2020
  network time: Fri, 15 May 2020 23:49:20 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72249/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72249/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (4488) (python)
##[section]Finishing: Finalize Job

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 @rust-lang/infra. (Feature Requests)


impl LangItemRecord {
/// Requires that the `LangItem` was bound and returns the corresponding `DefId`.
pub fn require(self) -> Result<DefId, LangItem> {
Copy link
Contributor

@ecstatic-morse ecstatic-morse May 16, 2020

Choose a reason for hiding this comment

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

Part of the reason to move away from Option is to make it hard for people to call unwrap. Having a function that returns a Result sort of defeats the purpose doesn't it? People will just write lang_items().sized().require().unwrap().

I think require should accept a TyCtxt and an Option<Span> and return DefId, emitting an error when it fails. If you want to do more complex things, you can match on LangItemRecord directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay yeah that’s a fair point - I’ll try again that way around although I feel like there may be some places where the code definitely take a hit

Copy link
Contributor

@ecstatic-morse ecstatic-morse May 16, 2020

Choose a reason for hiding this comment

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

To me, this indicates that this PR should go through the major-change proposal process as @nikomatsakis mentioned on the issue. MCP is relatively new, so I think we're still exploring what magnitude of change qualifies as "major". This PR is a good test of that lower bound.

Would you be interested in opening a proposal that explains the anti-pattern this PR tries to prevent as well as a rough sketch of the API? One positive side-effect of the process is that more people will be informed about the correct use of the new API.

Copy link
Contributor

Choose a reason for hiding this comment

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

A similar question-- is there a rustc-dev-guide entry on lang items? I think there should be, and I suspect that any such entry would be affected by this change (as it would likely have given some examples of how lang items are meant to be used).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think we have a dev guide entry on lang items yet and I agree that we should. I’ve got a couple of exams next week but once they’re done I’ll write up the MCP, rewrite this PR and start drafting some stuff for the dev guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great! Thanks for your work on this. =)

I don't think btw that the MCP has to be super detailed, I think sketching out the plan is sufficient, especially given that you can point at this PR. The main idea is to have some standard way to advertise changes people may want to be aware of.

/// Checks the current `LangItemRecord` against the given `DefId`.
/// Returns true if and only if the language item was found and its `DefId`
/// is equal to the `DefId` provided.
pub fn is(self, other: DefId) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider PartialEq<DefId> instead of a custom method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought about it but decided it seemed a little opaque - do you think PartialEq<DefId> would be better?

Copy link
Contributor

@ecstatic-morse ecstatic-morse May 16, 2020

Choose a reason for hiding this comment

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

I'm mostly indifferent but also lean towards an inherent method. Just wondering if you had a more convincing argument. FWIW, I would have chosen a different name (has_def_id), but that's just bikeshedding.

@@ -2731,6 +2731,6 @@ pub fn provide(providers: &mut ty::query::Providers<'_>) {
providers.has_panic_handler = |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
// We want to check if the panic handler was defined in this crate
tcx.lang_items().panic_impl().map_or(false, |did| did.is_local())
tcx.lang_items().panic_impl().require().map_or(false, |did| did.is_local())
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this could be a method on LangItemRecord, is_local or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s a really good point - thanks

.map(|t| chalk_ir::TraitId(RustDefId::Trait(t)))
.unwrap(),
CopyTrait => self
.tcx
.lang_items()
.copy_trait()
.require()
.map(|t| chalk_ir::TraitId(RustDefId::Trait(t)))
.unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the require().unwrap() pattern I was worried about BTW.

Copy link
Contributor Author

@doctorn doctorn May 16, 2020

Choose a reason for hiding this comment

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

Yeah I definitely agree - it’s just about trying to find a way to avoid having the whole compiler import the new type (I’m really not sure what the right answer is here)

Some(def_id) => def_id,
None => return None,
};
let sized_def_id = self.tcx.lang_items().sized_trait().require().ok()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we error in places like this when we don't have the Sized trait? I'm not sure if we really need to gracefully handle its absence.

@bors
Copy link
Contributor

bors commented May 16, 2020

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 16, 2020
@doctorn doctorn marked this pull request as draft May 29, 2020 15:55
@doctorn doctorn closed this Jun 3, 2020
@doctorn doctorn deleted the make-lang-items-priv branch June 8, 2020 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants