Some refactoring around intrinsic type checking #73834

oli-obk · 2020-06-28T11:02:34Z

So... This PR went a bit overboard. I wanted to make the rustc_peek intrinsic safe (cc @ecstatic-morse ), and remembered a long-standing itch of mine. So I made that huge &str match for the intrinsic name a match on Symbols (so basically u32s). This is unlikely to have a positive perf effect, even if it likely has better codegen (intrinsics are used rarely, mostly once in their wrapper), so it's mostly a consistency thing since other places actually match on the symbol name of the intrinsics.

rust-highfive · 2020-06-28T11:02:37Z

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

ecstatic-morse

r=me with glob imports removed and replaced with sym::xxx.

ecstatic-morse · 2020-06-28T16:23:19Z

src/librustc_typeck/check/intrinsic.rs

@@ -67,15 +67,17 @@ fn equate_intrinsic_type<'tcx>(
 }

 /// Returns `true` if the given intrinsic is unsafe to call or not.
-pub fn intrinsic_operation_unsafety(intrinsic: &str) -> hir::Unsafety {
+pub fn intrinsic_operation_unsafety(intrinsic: Symbol) -> hir::Unsafety {
+    use rustc_span::symbol::sym::*;


A glob import of sym pollutes the local namespace with a lot of lower-case constants. For this reason, it's not done anywhere else in the compiler. I realize that it's kind of a pain, but we should uphold this example. It's more relevant in the other big match statement, which has code outside the match.

yea... I knew that to be the right thing... but... laziness XD

ecstatic-morse · 2020-06-28T16:26:28Z

src/librustc_typeck/check/intrinsic.rs

@@ -303,7 +302,7 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {
                )
            }

-            "try" => {
+            rustc_span::symbol::kw::Try => {


Good catch. Having sym::Try and kw::Try mean different things is a bit subtle. I don't see how we could do better though.

oli-obk · 2020-06-28T18:45:44Z

@bors r=ecstatic-morse

bors · 2020-06-28T18:45:45Z

📌 Commit 824b2bb has been approved by ecstatic-morse

ecstatic-morse · 2020-06-28T23:56:04Z

@bors rollup

@ecstatic-morse

…-morse Some refactoring around intrinsic type checking So... This PR went a bit overboard. I wanted to make the `rustc_peek` intrinsic safe (cc @ecstatic-morse ), and remembered a long-standing itch of mine. So I made that huge `&str` match for the intrinsic name a match on `Symbol`s (so basically `u32`s). This is unlikely to have a positive perf effect, even if it likely has better codegen (intrinsics are used rarely, mostly once in their wrapper), so it's mostly a consistency thing since other places actually match on the symbol name of the intrinsics.

@ecstatic-morse

…-morse Some refactoring around intrinsic type checking So... This PR went a bit overboard. I wanted to make the `rustc_peek` intrinsic safe (cc @ecstatic-morse ), and remembered a long-standing itch of mine. So I made that huge `&str` match for the intrinsic name a match on `Symbol`s (so basically `u32`s). This is unlikely to have a positive perf effect, even if it likely has better codegen (intrinsics are used rarely, mostly once in their wrapper), so it's mostly a consistency thing since other places actually match on the symbol name of the intrinsics.

@ecstatic-morse

…-morse Some refactoring around intrinsic type checking So... This PR went a bit overboard. I wanted to make the `rustc_peek` intrinsic safe (cc @ecstatic-morse ), and remembered a long-standing itch of mine. So I made that huge `&str` match for the intrinsic name a match on `Symbol`s (so basically `u32`s). This is unlikely to have a positive perf effect, even if it likely has better codegen (intrinsics are used rarely, mostly once in their wrapper), so it's mostly a consistency thing since other places actually match on the symbol name of the intrinsics.

Manishearth · 2020-06-30T06:54:40Z

Is there a chance this PR is massively slowing down CI? It was a part of two rollups (#73880, #73864) that timed out, and while this is not the only suspect, it's worth investigating.

oli-obk · 2020-06-30T07:17:59Z

I didn't notice anything locally... but let's see

@bors try @rust-timer queue

rust-timer · 2020-06-30T07:18:00Z

Awaiting bors try build completion

bors · 2020-06-30T07:18:00Z

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

oli-obk · 2020-06-30T07:18:19Z

@bors r-

oli-obk · 2020-06-30T07:18:27Z

@bors try

oli-obk · 2020-07-02T07:37:54Z

@bors r+

It's not this PR, some other PR didn't rebase and bless. I'll figure out the problematic PR

bors · 2020-07-02T07:37:56Z

📌 Commit 824b2bb has been approved by oli-obk

oli-obk · 2020-07-02T07:40:09Z

I think it was https://github.com/rust-lang/rust/pull/73622/files#diff-c4db7f9b504f4fbce6ba0b53e7810379, I'll mention it in that PR

nevermind, that's not the one

oli-obk · 2020-07-02T07:45:20Z

I'll try to figure it out later, but some PR included a miri submodule update, even though it should not, idk which one yet.

oli-obk · 2020-07-02T08:03:30Z

ah it was #72983 which already knows about it

bors · 2020-07-02T16:47:06Z

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

oli-obk · 2020-07-04T16:39:12Z

@bors r=ecstatic-morse rollup

bors · 2020-07-04T16:39:13Z

📌 Commit 394b8cd has been approved by ecstatic-morse

@ecstatic-morse

…-morse Some refactoring around intrinsic type checking So... This PR went a bit overboard. I wanted to make the `rustc_peek` intrinsic safe (cc @ecstatic-morse ), and remembered a long-standing itch of mine. So I made that huge `&str` match for the intrinsic name a match on `Symbol`s (so basically `u32`s). This is unlikely to have a positive perf effect, even if it likely has better codegen (intrinsics are used rarely, mostly once in their wrapper), so it's mostly a consistency thing since other places actually match on the symbol name of the intrinsics.

@ecstatic-morse

…-morse Some refactoring around intrinsic type checking So... This PR went a bit overboard. I wanted to make the `rustc_peek` intrinsic safe (cc @ecstatic-morse ), and remembered a long-standing itch of mine. So I made that huge `&str` match for the intrinsic name a match on `Symbol`s (so basically `u32`s). This is unlikely to have a positive perf effect, even if it likely has better codegen (intrinsics are used rarely, mostly once in their wrapper), so it's mostly a consistency thing since other places actually match on the symbol name of the intrinsics.

@ghost

…arth Rollup of 12 pull requests Successful merges: - rust-lang#72688 (added .collect() into String from Box<str>) - rust-lang#73787 (Add unstable docs for rustc_attrs) - rust-lang#73834 (Some refactoring around intrinsic type checking) - rust-lang#73871 (Fix try_print_visible_def_path for Rust 2018) - rust-lang#73937 (Explain exhaustive matching on {usize,isize} maximum values) - rust-lang#73973 (Use `Span`s to identify unreachable subpatterns in or-patterns) - rust-lang#74000 (add `lazy_normalization_consts` feature gate) - rust-lang#74025 (Remove unnecessary release from Arc::try_unwrap) - rust-lang#74027 (Convert more `DefId`s to `LocalDefId`s) - rust-lang#74055 (Fix spacing in Iterator fold doc) - rust-lang#74057 (expected_found `&T` -> `T`) - rust-lang#74064 (variant_count: avoid incorrect dummy implementation) Failed merges: r? @ghost

rust-highfive assigned ecstatic-morse Jun 28, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2020

This comment has been minimized.

Sign in to view

oli-obk force-pushed the safe_intrinsics branch from b71a1a9 to 2b9f542 Compare June 28, 2020 12:29

ecstatic-morse reviewed Jun 28, 2020

View reviewed changes

oli-obk force-pushed the safe_intrinsics branch from 2b9f542 to 824b2bb Compare June 28, 2020 18:45

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 Jun 28, 2020

Manishearth mentioned this pull request Jun 29, 2020

Rollup of 12 pull requests #73864

Closed

Manishearth mentioned this pull request Jun 29, 2020

Rollup of 13 pull requests #73876

Closed

Manishearth mentioned this pull request Jun 30, 2020

Rollup of 13 pull requests #73880

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 30, 2020

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 1, 2020

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 Jul 2, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2020

oli-obk added 2 commits July 4, 2020 18:30

Make rustc_peek a safe intrinsic

66fb778

Match on Symbol instead of &str for type-checking intrinsics.

394b8cd

oli-obk force-pushed the safe_intrinsics branch from 824b2bb to 394b8cd Compare July 4, 2020 16:38

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 Jul 4, 2020

Manishearth mentioned this pull request Jul 4, 2020

Rollup of 5 pull requests #74039

Closed

Manishearth mentioned this pull request Jul 5, 2020

Rollup of 5 pull requests #74054

Closed

Manishearth mentioned this pull request Jul 5, 2020

Rollup of 12 pull requests #74073

Merged

bors merged commit fed2013 into rust-lang:master Jul 6, 2020

oli-obk deleted the safe_intrinsics branch March 16, 2021 12:14

clubby789 mentioned this pull request Jan 27, 2024

Incorrect types shown in an error when using rhai #120416

Open

cuviper added this to the 1.46 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some refactoring around intrinsic type checking #73834

Some refactoring around intrinsic type checking #73834

oli-obk commented Jun 28, 2020

rust-highfive commented Jun 28, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ecstatic-morse left a comment

ecstatic-morse Jun 28, 2020 •

edited

Loading

oli-obk Jun 28, 2020

ecstatic-morse Jun 28, 2020

oli-obk commented Jun 28, 2020

bors commented Jun 28, 2020

ecstatic-morse commented Jun 28, 2020

Manishearth commented Jun 30, 2020

oli-obk commented Jun 30, 2020

rust-timer commented Jun 30, 2020

bors commented Jun 30, 2020

oli-obk commented Jun 30, 2020

oli-obk commented Jun 30, 2020

oli-obk commented Jul 2, 2020

bors commented Jul 2, 2020

oli-obk commented Jul 2, 2020 •

edited

Loading

oli-obk commented Jul 2, 2020

oli-obk commented Jul 2, 2020

bors commented Jul 2, 2020

oli-obk commented Jul 4, 2020

bors commented Jul 4, 2020

Some refactoring around intrinsic type checking #73834

Some refactoring around intrinsic type checking #73834

Conversation

oli-obk commented Jun 28, 2020

rust-highfive commented Jun 28, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ecstatic-morse left a comment

Choose a reason for hiding this comment

ecstatic-morse Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

oli-obk Jun 28, 2020

Choose a reason for hiding this comment

ecstatic-morse Jun 28, 2020

Choose a reason for hiding this comment

oli-obk commented Jun 28, 2020

bors commented Jun 28, 2020

ecstatic-morse commented Jun 28, 2020

Manishearth commented Jun 30, 2020

oli-obk commented Jun 30, 2020

rust-timer commented Jun 30, 2020

bors commented Jun 30, 2020

oli-obk commented Jun 30, 2020

oli-obk commented Jun 30, 2020

oli-obk commented Jul 2, 2020

bors commented Jul 2, 2020

oli-obk commented Jul 2, 2020 • edited Loading

oli-obk commented Jul 2, 2020

oli-obk commented Jul 2, 2020

bors commented Jul 2, 2020

oli-obk commented Jul 4, 2020

bors commented Jul 4, 2020

ecstatic-morse Jun 28, 2020 •

edited

Loading

oli-obk commented Jul 2, 2020 •

edited

Loading