From c1d72de030f3c1287a363ff7cb134664a6bf3032 Mon Sep 17 00:00:00 2001 From: Michael Howell <michael@notriddle.com> Date: Tue, 23 May 2023 15:29:43 -0700 Subject: [PATCH 01/11] rustdoc: add interaction delays for tooltip popovers Designing a good hover microinteraction is a matter of guessing user intent from what are, literally, vague gestures. In this case, guessing if hovering in our out of the tooltip base is intentional or not. To figure this out, a few different techniques are used: * When the mouse pointer enters a tooltip anchor point, its hitbox is grown on the bottom, where the popover is/will appear. This was already there before this commit: search "hover tunnel" in rustdoc.css for the implementation. * This commit adds a delay when the mouse pointer enters the base anchor, in case the mouse pointer was just passing through and the user didn't want to open it. * This commit also adds a delay when the mouse pointer exits the tooltip's base anchor or its popover, before hiding it. * A fade-out animation is layered onto the pointer exit delay to immediately inform the user that they successfully dismissed the popover, while still providing a way for them to cancel it if it was a mistake and they still wanted to interact with it. * No animation is used for revealing it, because we don't want people to try to interact with an element while it's in the middle of fading in: either they're allowed to interact with it while it's fading in, meaning it can't serve as mistake- proofing for opening the popover, or they can't, but they might try and be frustrated. See also: * https://www.nngroup.com/articles/timing-exposing-content/ * https://www.nngroup.com/articles/tooltip-guidelines/ * https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown --- src/librustdoc/html/static/css/rustdoc.css | 8 ++ src/librustdoc/html/static/js/main.js | 113 +++++++++++++++++++-- tests/rustdoc-gui/codeblock-tooltip.goml | 2 + tests/rustdoc-gui/notable-trait.goml | 2 +- 4 files changed, 115 insertions(+), 10 deletions(-) diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index a7d5f497756..cd5ff48c979 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -1191,6 +1191,14 @@ a.tooltip:hover::after { content: "\00a0"; } +/* This animation is layered onto the mistake-proofing delay for dismissing + a hovered tooltip, to ensure it feels responsive even with the delay. + */ +.fade-out { + opacity: 0; + transition: opacity 0.45s cubic-bezier(0, 0, 0.1, 1.0); +} + .popover.tooltip .content { margin: 0.25em 0.5em; } diff --git a/src/librustdoc/html/static/js/main.js b/src/librustdoc/html/static/js/main.js index bccf675c14b..11ed2f5f901 100644 --- a/src/librustdoc/html/static/js/main.js +++ b/src/librustdoc/html/static/js/main.js @@ -4,6 +4,13 @@ "use strict"; +// The amount of time that the cursor must remain still over a hover target before +// revealing a tooltip. +// +// https://www.nngroup.com/articles/timing-exposing-content/ +window.RUSTDOC_TOOLTIP_HOVER_MS = 300; +window.RUSTDOC_TOOLTIP_HOVER_EXIT_MS = 450; + // Given a basename (e.g. "storage") and an extension (e.g. ".js"), return a URL // for a resource under the root-path, with the resource-suffix. function resourcePath(basename, extension) { @@ -784,6 +791,7 @@ function preLoadCss(cssUrl) { } if (window.CURRENT_TOOLTIP_ELEMENT && window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE === e) { // Make this function idempotent. + clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT); return; } window.hideAllModals(false); @@ -791,11 +799,17 @@ function preLoadCss(cssUrl) { if (notable_ty) { wrapper.innerHTML = "<div class=\"content\">" + window.NOTABLE_TRAITS[notable_ty] + "</div>"; - } else if (e.getAttribute("title") !== undefined) { - const titleContent = document.createElement("div"); - titleContent.className = "content"; - titleContent.appendChild(document.createTextNode(e.getAttribute("title"))); - wrapper.appendChild(titleContent); + } else { + if (e.getAttribute("title") !== null) { + e.setAttribute("data-title", e.getAttribute("title")); + e.removeAttribute("title"); + } + if (e.getAttribute("data-title") !== null) { + const titleContent = document.createElement("div"); + titleContent.className = "content"; + titleContent.appendChild(document.createTextNode(e.getAttribute("data-title"))); + wrapper.appendChild(titleContent); + } } wrapper.className = "tooltip popover"; const focusCatcher = document.createElement("div"); @@ -824,17 +838,59 @@ function preLoadCss(cssUrl) { wrapper.style.visibility = ""; window.CURRENT_TOOLTIP_ELEMENT = wrapper; window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE = e; + clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT); + wrapper.onpointerenter = function(ev) { + // If this is a synthetic touch event, ignore it. A click event will be along shortly. + if (ev.pointerType !== "mouse") { + return; + } + clearTooltipHoverTimeout(e); + }; wrapper.onpointerleave = function(ev) { // If this is a synthetic touch event, ignore it. A click event will be along shortly. if (ev.pointerType !== "mouse") { return; } - if (!e.TOOLTIP_FORCE_VISIBLE && !elemIsInParent(event.relatedTarget, e)) { - hideTooltip(true); + if (!e.TOOLTIP_FORCE_VISIBLE && !elemIsInParent(ev.relatedTarget, e)) { + // See "Tooltip pointer leave gesture" below. + setTooltipHoverTimeout(e, false); + addClass(wrapper, "fade-out"); } }; } + function setTooltipHoverTimeout(element, show) { + clearTooltipHoverTimeout(element); + if (!show && !window.CURRENT_TOOLTIP_ELEMENT) { + // To "hide" an already hidden element, just cancel its timeout. + return; + } + if (show && window.CURRENT_TOOLTIP_ELEMENT) { + // To "show" an already visible element, just cancel its timeout. + return; + } + if (window.CURRENT_TOOLTIP_ELEMENT && + window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE !== element) { + // Don't do anything if another tooltip is already visible. + return; + } + element.TOOLTIP_HOVER_TIMEOUT = setTimeout(() => { + if (show) { + showTooltip(element); + } else if (!element.TOOLTIP_FORCE_VISIBLE) { + hideTooltip(false); + } + }, show ? window.RUSTDOC_TOOLTIP_HOVER_MS : window.RUSTDOC_TOOLTIP_HOVER_EXIT_MS); + } + + function clearTooltipHoverTimeout(element) { + if (element.TOOLTIP_HOVER_TIMEOUT !== undefined) { + removeClass(window.CURRENT_TOOLTIP_ELEMENT, "fade-out"); + clearTimeout(element.TOOLTIP_HOVER_TIMEOUT); + delete element.TOOLTIP_HOVER_TIMEOUT; + } + } + function tooltipBlurHandler(event) { if (window.CURRENT_TOOLTIP_ELEMENT && !elemIsInParent(document.activeElement, window.CURRENT_TOOLTIP_ELEMENT) && @@ -864,6 +920,7 @@ function preLoadCss(cssUrl) { } const body = document.getElementsByTagName("body")[0]; body.removeChild(window.CURRENT_TOOLTIP_ELEMENT); + clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT); window.CURRENT_TOOLTIP_ELEMENT = null; } } @@ -886,7 +943,14 @@ function preLoadCss(cssUrl) { if (ev.pointerType !== "mouse") { return; } - showTooltip(this); + setTooltipHoverTimeout(this, true); + }; + e.onpointermove = function(ev) { + // If this is a synthetic touch event, ignore it. A click event will be along shortly. + if (ev.pointerType !== "mouse") { + return; + } + setTooltipHoverTimeout(this, true); }; e.onpointerleave = function(ev) { // If this is a synthetic touch event, ignore it. A click event will be along shortly. @@ -895,7 +959,38 @@ function preLoadCss(cssUrl) { } if (!this.TOOLTIP_FORCE_VISIBLE && !elemIsInParent(ev.relatedTarget, window.CURRENT_TOOLTIP_ELEMENT)) { - hideTooltip(true); + // Tooltip pointer leave gesture: + // + // Designing a good hover microinteraction is a matter of guessing user + // intent from what are, literally, vague gestures. In this case, guessing if + // hovering in or out of the tooltip base is intentional or not. + // + // To figure this out, a few different techniques are used: + // + // * When the mouse pointer enters a tooltip anchor point, its hitbox is grown + // on the bottom, where the popover is/will appear. Search "hover tunnel" in + // rustdoc.css for the implementation. + // * There's a delay when the mouse pointer enters the popover base anchor, in + // case the mouse pointer was just passing through and the user didn't want + // to open it. + // * Similarly, a delay is added when exiting the anchor, or the popover + // itself, before hiding it. + // * A fade-out animation is layered onto the pointer exit delay to immediately + // inform the user that they successfully dismissed the popover, while still + // providing a way for them to cancel it if it was a mistake and they still + // wanted to interact with it. + // * No animation is used for revealing it, because we don't want people to try + // to interact with an element while it's in the middle of fading in: either + // they're allowed to interact with it while it's fading in, meaning it can't + // serve as mistake-proofing for the popover, or they can't, but + // they might try and be frustrated. + // + // See also: + // * https://www.nngroup.com/articles/timing-exposing-content/ + // * https://www.nngroup.com/articles/tooltip-guidelines/ + // * https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown + setTooltipHoverTimeout(e, false); + addClass(window.CURRENT_TOOLTIP_ELEMENT, "fade-out"); } }; }); diff --git a/tests/rustdoc-gui/codeblock-tooltip.goml b/tests/rustdoc-gui/codeblock-tooltip.goml index e1c81ed79e4..7be5e39ba47 100644 --- a/tests/rustdoc-gui/codeblock-tooltip.goml +++ b/tests/rustdoc-gui/codeblock-tooltip.goml @@ -40,6 +40,7 @@ define-function: ( "background-color": |background|, "border-color": |border|, }) + click: ".docblock .example-wrap.compile_fail .tooltip" // should_panic block assert-css: ( @@ -71,6 +72,7 @@ define-function: ( "background-color": |background|, "border-color": |border|, }) + click: ".docblock .example-wrap.should_panic .tooltip" // ignore block assert-css: ( diff --git a/tests/rustdoc-gui/notable-trait.goml b/tests/rustdoc-gui/notable-trait.goml index ecb57c274a5..09b3dfe2825 100644 --- a/tests/rustdoc-gui/notable-trait.goml +++ b/tests/rustdoc-gui/notable-trait.goml @@ -134,7 +134,7 @@ define-function: ( reload: move-cursor-to: "//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']" - assert-count: (".tooltip.popover", 1) + wait-for-count: (".tooltip.popover", 1) assert-css: ( ".tooltip.popover h3", From b9302d72346444b74bfbb2274c6da1acda9622a7 Mon Sep 17 00:00:00 2001 From: Michael Howell <michael@notriddle.com> Date: Tue, 23 May 2023 17:19:35 -0700 Subject: [PATCH 02/11] rustdoc: add hover indicator for notable trait tooltip --- src/librustdoc/html/static/css/rustdoc.css | 4 ++++ tests/rustdoc-gui/notable-trait.goml | 17 ++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index cd5ff48c979..054cfe7597e 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -1179,6 +1179,10 @@ a.test-arrow:hover { position: relative; } +.code-header a.tooltip:hover { + color: var(--link-color); +} + /* placeholder thunk so that the mouse can easily travel from "(i)" to popover the resulting "hover tunnel" is a stepped triangle, approximating https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown */ diff --git a/tests/rustdoc-gui/notable-trait.goml b/tests/rustdoc-gui/notable-trait.goml index 09b3dfe2825..371931d51fc 100644 --- a/tests/rustdoc-gui/notable-trait.goml +++ b/tests/rustdoc-gui/notable-trait.goml @@ -122,7 +122,7 @@ assert-count: ("//*[@class='tooltip popover']", 0) // Now check the colors. define-function: ( "check-colors", - (theme, header_color, content_color, type_color, trait_color), + (theme, header_color, content_color, type_color, trait_color, link_color), block { go-to: "file://" + |DOC_PATH| + "/test_docs/struct.NotableStructWithLongName.html" // This is needed to ensure that the text color is computed. @@ -133,9 +133,21 @@ define-function: ( // We reload the page so the local storage settings are being used. reload: + assert-css: ( + "//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']", + {"color": |content_color|}, + ALL, + ) + move-cursor-to: "//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']" wait-for-count: (".tooltip.popover", 1) + assert-css: ( + "//*[@id='method.create_an_iterator_from_read']//*[@class='tooltip']", + {"color": |link_color|}, + ALL, + ) + assert-css: ( ".tooltip.popover h3", {"color": |header_color|}, @@ -163,6 +175,7 @@ call-function: ( "check-colors", { "theme": "ayu", + "link_color": "rgb(57, 175, 215)", "content_color": "rgb(230, 225, 207)", "header_color": "rgb(255, 255, 255)", "type_color": "rgb(255, 160, 165)", @@ -174,6 +187,7 @@ call-function: ( "check-colors", { "theme": "dark", + "link_color": "rgb(210, 153, 29)", "content_color": "rgb(221, 221, 221)", "header_color": "rgb(221, 221, 221)", "type_color": "rgb(45, 191, 184)", @@ -185,6 +199,7 @@ call-function: ( "check-colors", { "theme": "light", + "link_color": "rgb(56, 115, 173)", "content_color": "rgb(0, 0, 0)", "header_color": "rgb(0, 0, 0)", "type_color": "rgb(173, 55, 138)", From a810b584cf7bf6f3d0eb02cef441b510475514f9 Mon Sep 17 00:00:00 2001 From: Michael Goulet <michael@errs.io> Date: Thu, 25 May 2023 20:32:11 +0000 Subject: [PATCH 03/11] Use DefiningAnchor::Bind in infer_opaque_definition_from_instantiation --- .../src/region_infer/opaque_types.rs | 14 ++++++++++++-- tests/ui/type-alias-impl-trait/cross_inference.rs | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_borrowck/src/region_infer/opaque_types.rs b/compiler/rustc_borrowck/src/region_infer/opaque_types.rs index 7fc89e89a35..725ff783ee9 100644 --- a/compiler/rustc_borrowck/src/region_infer/opaque_types.rs +++ b/compiler/rustc_borrowck/src/region_infer/opaque_types.rs @@ -278,8 +278,18 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> { // HACK This bubble is required for this tests to pass: // nested-return-type2-tait2.rs // nested-return-type2-tait3.rs - let infcx = - self.tcx.infer_ctxt().with_opaque_type_inference(DefiningAnchor::Bubble).build(); + // FIXME(-Ztrait-solver=next): We probably should use `DefiningAnchor::Error` + // and prepopulate this `InferCtxt` with known opaque values, rather than + // using the `Bind` anchor here. For now it's fine. + let infcx = self + .tcx + .infer_ctxt() + .with_opaque_type_inference(if self.tcx.trait_solver_next() { + DefiningAnchor::Bind(def_id) + } else { + DefiningAnchor::Bubble + }) + .build(); let ocx = ObligationCtxt::new(&infcx); // Require the hidden type to be well-formed with only the generics of the opaque type. // Defining use functions may have more bounds than the opaque type, which is ok, as long as the diff --git a/tests/ui/type-alias-impl-trait/cross_inference.rs b/tests/ui/type-alias-impl-trait/cross_inference.rs index dafaf40a69d..07f3dd1997b 100644 --- a/tests/ui/type-alias-impl-trait/cross_inference.rs +++ b/tests/ui/type-alias-impl-trait/cross_inference.rs @@ -1,3 +1,5 @@ +// revisions: current next +//[next] compile-flags: -Ztrait-solver=next // check-pass #![feature(type_alias_impl_trait)] From 3d09b990d7e3d8539a3b9655894e7e05dcc9ebf2 Mon Sep 17 00:00:00 2001 From: Michael Goulet <michael@errs.io> Date: Fri, 26 May 2023 03:47:27 +0000 Subject: [PATCH 04/11] Wait until type_of to remap HIR opaques back to their defn params --- compiler/rustc_borrowck/src/type_check/mod.rs | 8 +- .../src/collect/type_of/opaque.rs | 94 +++++++++++++------ compiler/rustc_hir_typeck/src/writeback.rs | 12 +-- .../rustc_middle/src/ty/typeck_results.rs | 2 +- .../new-solver/dont-remap-tait-substs.rs | 19 ++++ .../multiple-def-uses-in-one-fn.rs | 1 - .../multiple-def-uses-in-one-fn.stderr | 11 +-- 7 files changed, 92 insertions(+), 55 deletions(-) create mode 100644 tests/ui/traits/new-solver/dont-remap-tait-substs.rs diff --git a/compiler/rustc_borrowck/src/type_check/mod.rs b/compiler/rustc_borrowck/src/type_check/mod.rs index 51a84ce6cad..b759a848bf5 100644 --- a/compiler/rustc_borrowck/src/type_check/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/mod.rs @@ -188,9 +188,6 @@ pub(crate) fn type_check<'mir, 'tcx>( // FIXME(-Ztrait-solver=next): A bit dubious that we're only registering // predefined opaques in the typeck root. - // FIXME(-Ztrait-solver=next): This is also totally wrong for TAITs, since - // the HIR typeck map defining usages back to their definition params, - // they won't actually match up with the usages in this body... if infcx.tcx.trait_solver_next() && !infcx.tcx.is_typeck_child(body.source.def_id()) { checker.register_predefined_opaques_in_new_solver(); } @@ -1042,10 +1039,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { .typeck(self.body.source.def_id().expect_local()) .concrete_opaque_types .iter() - .map(|(&def_id, &hidden_ty)| { - let substs = ty::InternalSubsts::identity_for_item(self.infcx.tcx, def_id); - (ty::OpaqueTypeKey { def_id, substs }, hidden_ty) - }) + .map(|(k, v)| (*k, *v)) .collect(); let renumbered_opaques = self.infcx.tcx.fold_regions(opaques, |_, _| { diff --git a/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs b/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs index f7c5b44678f..da82ce1d523 100644 --- a/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs +++ b/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs @@ -59,7 +59,20 @@ pub(super) fn find_opaque_ty_constraints_for_tait(tcx: TyCtxt<'_>, def_id: Local } } - let Some(hidden) = locator.found else { + if let Some(hidden) = locator.found { + // Only check against typeck if we didn't already error + if !hidden.ty.references_error() { + for concrete_type in locator.typeck_types { + if concrete_type.ty != tcx.erase_regions(hidden.ty) + && !(concrete_type, hidden).references_error() + { + hidden.report_mismatch(&concrete_type, def_id, tcx).emit(); + } + } + } + + hidden.ty + } else { let reported = tcx.sess.emit_err(UnconstrainedOpaqueType { span: tcx.def_span(def_id), name: tcx.item_name(tcx.local_parent(def_id).to_def_id()), @@ -70,21 +83,8 @@ pub(super) fn find_opaque_ty_constraints_for_tait(tcx: TyCtxt<'_>, def_id: Local _ => "item", }, }); - return tcx.ty_error(reported); - }; - - // Only check against typeck if we didn't already error - if !hidden.ty.references_error() { - for concrete_type in locator.typeck_types { - if concrete_type.ty != tcx.erase_regions(hidden.ty) - && !(concrete_type, hidden).references_error() - { - hidden.report_mismatch(&concrete_type, def_id, tcx).emit(); - } - } + tcx.ty_error(reported) } - - hidden.ty } struct TaitConstraintLocator<'tcx> { @@ -130,13 +130,28 @@ impl TaitConstraintLocator<'_> { self.found = Some(ty::OpaqueHiddenType { span: DUMMY_SP, ty: self.tcx.ty_error(guar) }); return; } - let Some(&typeck_hidden_ty) = tables.concrete_opaque_types.get(&self.def_id) else { + + let mut constrained = false; + for (&opaque_type_key, &hidden_type) in &tables.concrete_opaque_types { + if opaque_type_key.def_id != self.def_id { + continue; + } + constrained = true; + let concrete_type = + self.tcx.erase_regions(hidden_type.remap_generic_params_to_declaration_params( + opaque_type_key, + self.tcx, + true, + )); + if self.typeck_types.iter().all(|prev| prev.ty != concrete_type.ty) { + self.typeck_types.push(concrete_type); + } + } + + if !constrained { debug!("no constraints in typeck results"); return; }; - if self.typeck_types.iter().all(|prev| prev.ty != typeck_hidden_ty.ty) { - self.typeck_types.push(typeck_hidden_ty); - } // Use borrowck to get the type with unerased regions. let concrete_opaque_types = &self.tcx.mir_borrowck(item_def_id).concrete_opaque_types; @@ -190,8 +205,8 @@ impl<'tcx> intravisit::Visitor<'tcx> for TaitConstraintLocator<'tcx> { } } -pub(super) fn find_opaque_ty_constraints_for_rpit( - tcx: TyCtxt<'_>, +pub(super) fn find_opaque_ty_constraints_for_rpit<'tcx>( + tcx: TyCtxt<'tcx>, def_id: LocalDefId, owner_def_id: LocalDefId, ) -> Ty<'_> { @@ -208,17 +223,40 @@ pub(super) fn find_opaque_ty_constraints_for_rpit( Node::TraitItem(it) => intravisit::walk_trait_item(&mut locator, it), other => bug!("{:?} is not a valid scope for an opaque type item", other), } - } - concrete.map(|concrete| concrete.ty).unwrap_or_else(|| { - let table = tcx.typeck(owner_def_id); - if let Some(guar) = table.tainted_by_errors { + concrete.ty + } else { + let tables = tcx.typeck(owner_def_id); + if let Some(guar) = tables.tainted_by_errors { // Some error in the // owner fn prevented us from populating // the `concrete_opaque_types` table. tcx.ty_error(guar) } else { - table.concrete_opaque_types.get(&def_id).map(|ty| ty.ty).unwrap_or_else(|| { + // Fall back to the RPIT we inferred during HIR typeck + let mut opaque_ty: Option<ty::OpaqueHiddenType<'tcx>> = None; + for (&opaque_type_key, &hidden_type) in &tables.concrete_opaque_types { + if opaque_type_key.def_id != def_id { + continue; + } + let concrete_type = + tcx.erase_regions(hidden_type.remap_generic_params_to_declaration_params( + opaque_type_key, + tcx, + true, + )); + if let Some(prev) = &mut opaque_ty { + if concrete_type.ty != prev.ty && !(concrete_type, prev.ty).references_error() { + prev.report_mismatch(&concrete_type, def_id, tcx).emit(); + } + } else { + opaque_ty = Some(concrete_type); + } + } + + if let Some(opaque_ty) = opaque_ty { + opaque_ty.ty + } else { // We failed to resolve the opaque type or it // resolves to itself. We interpret this as the // no values of the hidden type ever being constructed, @@ -226,9 +264,9 @@ pub(super) fn find_opaque_ty_constraints_for_rpit( // For backwards compatibility reasons, we fall back to // `()` until we the diverging default is changed. tcx.mk_diverging_default() - }) + } } - }) + } } struct RpitConstraintChecker<'tcx> { diff --git a/compiler/rustc_hir_typeck/src/writeback.rs b/compiler/rustc_hir_typeck/src/writeback.rs index 0f21fc1e662..964acc4eb77 100644 --- a/compiler/rustc_hir_typeck/src/writeback.rs +++ b/compiler/rustc_hir_typeck/src/writeback.rs @@ -583,19 +583,15 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { continue; } - let hidden_type = - self.tcx().erase_regions(hidden_type.remap_generic_params_to_declaration_params( - opaque_type_key, - self.tcx(), - true, - )); - + // Here we only detect impl trait definition conflicts when they + // are equal modulo regions. if let Some(last_opaque_ty) = self .typeck_results .concrete_opaque_types - .insert(opaque_type_key.def_id, hidden_type) + .insert(opaque_type_key, hidden_type) && last_opaque_ty.ty != hidden_type.ty { + assert!(!self.tcx().trait_solver_next()); hidden_type .report_mismatch(&last_opaque_ty, opaque_type_key.def_id, self.tcx()) .stash( diff --git a/compiler/rustc_middle/src/ty/typeck_results.rs b/compiler/rustc_middle/src/ty/typeck_results.rs index e04dbbff9a7..68bae5828c5 100644 --- a/compiler/rustc_middle/src/ty/typeck_results.rs +++ b/compiler/rustc_middle/src/ty/typeck_results.rs @@ -159,7 +159,7 @@ pub struct TypeckResults<'tcx> { /// These types are mapped back to the opaque's identity substitutions /// (with erased regions), which is why we don't associated substs with any /// of these usages. - pub concrete_opaque_types: FxIndexMap<LocalDefId, ty::OpaqueHiddenType<'tcx>>, + pub concrete_opaque_types: FxIndexMap<ty::OpaqueTypeKey<'tcx>, ty::OpaqueHiddenType<'tcx>>, /// Tracks the minimum captures required for a closure; /// see `MinCaptureInformationMap` for more details. diff --git a/tests/ui/traits/new-solver/dont-remap-tait-substs.rs b/tests/ui/traits/new-solver/dont-remap-tait-substs.rs new file mode 100644 index 00000000000..028222f4e6d --- /dev/null +++ b/tests/ui/traits/new-solver/dont-remap-tait-substs.rs @@ -0,0 +1,19 @@ +// compile-flags: -Ztrait-solver=next +// check-pass + +// Makes sure we don't prepopulate the MIR typeck of `define` +// with `Foo<T, U> = T`, but instead, `Foo<B, A> = B`, so that +// the param-env predicates actually apply. + +#![feature(type_alias_impl_trait)] + +type Foo<T: Send, U> = impl NeedsSend<T>; + +trait NeedsSend<T> {} +impl<T: Send> NeedsSend<T> for T {} + +fn define<A, B: Send>(a: A, b: B) { + let y: Option<Foo<B, A>> = Some(b); +} + +fn main() {} diff --git a/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.rs b/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.rs index 9ae2c34b935..da845e86147 100644 --- a/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.rs +++ b/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.rs @@ -8,7 +8,6 @@ type X<A, B> = impl Into<&'static A>; fn f<A, B: 'static>(a: &'static A, b: B) -> (X<A, B>, X<B, A>) { //~^ ERROR the trait bound `&'static B: From<&A>` is not satisfied - //~| ERROR concrete type differs from previous defining opaque type use (a, a) } diff --git a/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr b/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr index 0d24d42ba62..66a6b0bbf74 100644 --- a/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr +++ b/tests/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr @@ -10,15 +10,6 @@ help: consider introducing a `where` clause, but there might be an alternative b LL | fn f<A, B: 'static>(a: &'static A, b: B) -> (X<A, B>, X<B, A>) where &'static B: From<&A> { | ++++++++++++++++++++++++++ -error: concrete type differs from previous defining opaque type use - --> $DIR/multiple-def-uses-in-one-fn.rs:9:45 - | -LL | fn f<A, B: 'static>(a: &'static A, b: B) -> (X<A, B>, X<B, A>) { - | ^^^^^^^^^^^^^^^^^^ - | | - | expected `&B`, got `&A` - | this expression supplies two conflicting concrete types for the same opaque type - -error: aborting due to 2 previous errors +error: aborting due to previous error For more information about this error, try `rustc --explain E0277`. From 9968f3ce55d83baf916d55920b721d737581efc7 Mon Sep 17 00:00:00 2001 From: benediktwerner <1benediktwerner@gmail.com> Date: Sat, 25 Feb 2023 17:52:25 +0100 Subject: [PATCH 05/11] rustdoc: Fix LinkReplacer link matching --- src/librustdoc/html/markdown.rs | 15 ++++++--- tests/rustdoc/intra-doc/issue-108459.rs | 37 ++++++++++++++++++++++ tests/rustdoc/intra-doc/prim-precedence.rs | 2 +- 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 tests/rustdoc/intra-doc/issue-108459.rs diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 9ef0b501c08..2f7ffce017b 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -382,7 +382,6 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, I> { Some(Event::Code(text)) => { trace!("saw code {}", text); if let Some(link) = self.shortcut_link { - trace!("original text was {}", link.original_text); // NOTE: this only replaces if the code block is the *entire* text. // If only part of the link has code highlighting, the disambiguator will not be removed. // e.g. [fn@`f`] @@ -391,8 +390,11 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, I> { // So we could never be sure we weren't replacing too much: // [fn@my_`f`unc] is treated the same as [my_func()] in that pass. // - // NOTE: &[1..len() - 1] is to strip the backticks - if **text == link.original_text[1..link.original_text.len() - 1] { + // NOTE: .get(1..len() - 1) is to strip the backticks + if let Some(link) = self.links.iter().find(|l| { + l.href == link.href + && Some(&**text) == l.original_text.get(1..l.original_text.len() - 1) + }) { debug!("replacing {} with {}", text, link.new_text); *text = CowStr::Borrowed(&link.new_text); } @@ -403,9 +405,12 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, I> { Some(Event::Text(text)) => { trace!("saw text {}", text); if let Some(link) = self.shortcut_link { - trace!("original text was {}", link.original_text); // NOTE: same limitations as `Event::Code` - if **text == *link.original_text { + if let Some(link) = self + .links + .iter() + .find(|l| l.href == link.href && **text == *l.original_text) + { debug!("replacing {} with {}", text, link.new_text); *text = CowStr::Borrowed(&link.new_text); } diff --git a/tests/rustdoc/intra-doc/issue-108459.rs b/tests/rustdoc/intra-doc/issue-108459.rs new file mode 100644 index 00000000000..eb1c7a05e54 --- /dev/null +++ b/tests/rustdoc/intra-doc/issue-108459.rs @@ -0,0 +1,37 @@ +#![deny(rustdoc::broken_intra_doc_links)] + +pub struct S; +pub mod char {} + +// Ensure this doesn't ICE due to trying to slice off non-existent backticks from "S" + +/// See [S] and [`S`] +pub struct MyStruct1; + +// Ensure that link texts are replaced correctly even if there are multiple links with +// the same target but different text + +/// See also [crate::char] and [mod@char] and [prim@char] +// @has issue_108459/struct.MyStruct2.html '//*[@href="char/index.html"]' 'crate::char' +// @has - '//*[@href="char/index.html"]' 'char' +// @has - '//*[@href="{{channel}}/std/primitive.char.html"]' 'char' +pub struct MyStruct2; + +/// See also [mod@char] and [prim@char] and [crate::char] +// @has issue_108459/struct.MyStruct3.html '//*[@href="char/index.html"]' 'crate::char' +// @has - '//*[@href="char/index.html"]' 'char' +// @has - '//*[@href="{{channel}}/std/primitive.char.html"]' 'char' +pub struct MyStruct3; + +// Ensure that links are correct even if there are multiple links with the same text but +// different targets + +/// See also [char][mod@char] and [char][prim@char] +// @has issue_108459/struct.MyStruct4.html '//*[@href="char/index.html"]' 'char' +// @has - '//*[@href="{{channel}}/std/primitive.char.html"]' 'char' +pub struct MyStruct4; + +/// See also [char][prim@char] and [char][crate::char] +// @has issue_108459/struct.MyStruct5.html '//*[@href="char/index.html"]' 'char' +// @has - '//*[@href="{{channel}}/std/primitive.char.html"]' 'char' +pub struct MyStruct5; diff --git a/tests/rustdoc/intra-doc/prim-precedence.rs b/tests/rustdoc/intra-doc/prim-precedence.rs index 25625b95277..c5a64e42a01 100644 --- a/tests/rustdoc/intra-doc/prim-precedence.rs +++ b/tests/rustdoc/intra-doc/prim-precedence.rs @@ -12,5 +12,5 @@ pub struct MyString; /// See also [crate::char] and [mod@char] // @has prim_precedence/struct.MyString2.html '//*[@href="char/index.html"]' 'crate::char' -// @has - '//*[@href="char/index.html"]' 'mod@char' +// @has - '//*[@href="char/index.html"]' 'char' pub struct MyString2; From cc21d9aa522cf07148e154ce9aec5f5f7a67c86c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote <n.nethercote@gmail.com> Date: Wed, 31 May 2023 14:42:54 +1000 Subject: [PATCH 06/11] Don't compute inlining status of mono items in advance. We record inlining status for mono items in `MonoItems`, and then transfer it to `InliningMap`, for later use in `InliningMap::with_inlining_candidates`. But we can just compute inlining status directly in `InliningMap::with_inlining_candidates`, because the mono item is right there. There's no need to compute it in advance. This commit changes the code to do that, removing the need for `MonoItems` and `InliningMap::inlines`. This does result in more calls to `instantiation_mode` (one per static occurrence) but the performance effect is negligible. --- compiler/rustc_monomorphize/src/collector.rs | 76 ++++--------------- .../rustc_monomorphize/src/partitioning.rs | 7 +- 2 files changed, 17 insertions(+), 66 deletions(-) diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index cefa64d27ac..09025c1ee33 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -179,7 +179,6 @@ use rustc_hir as hir; use rustc_hir::def::DefKind; use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId}; use rustc_hir::lang_items::LangItem; -use rustc_index::bit_set::GrowableBitSet; use rustc_middle::mir::interpret::{AllocId, ConstValue}; use rustc_middle::mir::interpret::{ErrorHandled, GlobalAlloc, Scalar}; use rustc_middle::mir::mono::{InstantiationMode, MonoItem}; @@ -220,78 +219,29 @@ pub struct InliningMap<'tcx> { // The range selects elements within the `targets` vecs. index: FxHashMap<MonoItem<'tcx>, Range<usize>>, targets: Vec<MonoItem<'tcx>>, - - // Contains one bit per mono item in the `targets` field. That bit - // is true if that mono item needs to be inlined into every CGU. - inlines: GrowableBitSet<usize>, } -/// Struct to store mono items in each collecting and if they should -/// be inlined. We call `instantiation_mode` to get their inlining -/// status when inserting new elements, which avoids calling it in -/// `inlining_map.lock_mut()`. See the `collect_items_rec` implementation -/// below. -struct MonoItems<'tcx> { - // If this is false, we do not need to compute whether items - // will need to be inlined. - compute_inlining: bool, - - // The TyCtxt used to determine whether the a item should - // be inlined. - tcx: TyCtxt<'tcx>, - - // The collected mono items. The bool field in each element - // indicates whether this element should be inlined. - items: Vec<(Spanned<MonoItem<'tcx>>, bool /*inlined*/)>, -} - -impl<'tcx> MonoItems<'tcx> { - #[inline] - fn push(&mut self, item: Spanned<MonoItem<'tcx>>) { - self.extend([item]); - } - - #[inline] - fn extend<T: IntoIterator<Item = Spanned<MonoItem<'tcx>>>>(&mut self, iter: T) { - self.items.extend(iter.into_iter().map(|mono_item| { - let inlined = if !self.compute_inlining { - false - } else { - mono_item.node.instantiation_mode(self.tcx) == InstantiationMode::LocalCopy - }; - (mono_item, inlined) - })) - } -} +type MonoItems<'tcx> = Vec<Spanned<MonoItem<'tcx>>>; impl<'tcx> InliningMap<'tcx> { fn new() -> InliningMap<'tcx> { - InliningMap { - index: FxHashMap::default(), - targets: Vec::new(), - inlines: GrowableBitSet::with_capacity(1024), - } + InliningMap { index: FxHashMap::default(), targets: Vec::new() } } fn record_accesses<'a>( &mut self, source: MonoItem<'tcx>, - new_targets: &'a [(Spanned<MonoItem<'tcx>>, bool)], + new_targets: &'a [Spanned<MonoItem<'tcx>>], ) where 'tcx: 'a, { let start_index = self.targets.len(); let new_items_count = new_targets.len(); - let new_items_count_total = new_items_count + self.targets.len(); self.targets.reserve(new_items_count); - self.inlines.ensure(new_items_count_total); - for (i, (Spanned { node: mono_item, .. }, inlined)) in new_targets.into_iter().enumerate() { + for Spanned { node: mono_item, .. } in new_targets.into_iter() { self.targets.push(*mono_item); - if *inlined { - self.inlines.insert(i + start_index); - } } let end_index = self.targets.len(); @@ -300,13 +250,14 @@ impl<'tcx> InliningMap<'tcx> { /// Internally iterate over all items referenced by `source` which will be /// made available for inlining. - pub fn with_inlining_candidates<F>(&self, source: MonoItem<'tcx>, mut f: F) + pub fn with_inlining_candidates<F>(&self, tcx: TyCtxt<'tcx>, source: MonoItem<'tcx>, mut f: F) where F: FnMut(MonoItem<'tcx>), { if let Some(range) = self.index.get(&source) { - for (i, candidate) in self.targets[range.clone()].iter().enumerate() { - if self.inlines.contains(range.start + i) { + for candidate in self.targets[range.clone()].iter() { + let is_inlined = candidate.instantiation_mode(tcx) == InstantiationMode::LocalCopy; + if is_inlined { f(*candidate); } } @@ -367,7 +318,7 @@ pub fn collect_crate_mono_items( #[instrument(skip(tcx, mode), level = "debug")] fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec<MonoItem<'_>> { debug!("collecting roots"); - let mut roots = MonoItems { compute_inlining: false, tcx, items: Vec::new() }; + let mut roots = Vec::new(); { let entry_fn = tcx.entry_fn(()); @@ -393,9 +344,8 @@ fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec<MonoItem< // whose predicates hold. Luckily, items that aren't instantiable // can't actually be used, so we can just skip codegenning them. roots - .items .into_iter() - .filter_map(|(Spanned { node: mono_item, .. }, _)| { + .filter_map(|Spanned { node: mono_item, .. }| { mono_item.is_instantiable(tcx).then_some(mono_item) }) .collect() @@ -417,7 +367,7 @@ fn collect_items_rec<'tcx>( return; } - let mut neighbors = MonoItems { compute_inlining: true, tcx, items: Vec::new() }; + let mut neighbors = Vec::new(); let recursion_depth_reset; // @@ -542,9 +492,9 @@ fn collect_items_rec<'tcx>( formatted_item, }); } - inlining_map.lock_mut().record_accesses(starting_point.node, &neighbors.items); + inlining_map.lock_mut().record_accesses(starting_point.node, &neighbors); - for (neighbour, _) in neighbors.items { + for neighbour in neighbors { collect_items_rec(tcx, neighbour, visited, recursion_depths, recursion_limit, inlining_map); } diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index be9c349c384..0bacb58383d 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -424,7 +424,7 @@ fn place_inlined_mono_items<'tcx>( // Collect all items that need to be available in this codegen unit. let mut reachable = FxHashSet::default(); for root in old_codegen_unit.items().keys() { - follow_inlining(*root, cx.inlining_map, &mut reachable); + follow_inlining(cx.tcx, *root, cx.inlining_map, &mut reachable); } let mut new_codegen_unit = CodegenUnit::new(old_codegen_unit.name()); @@ -478,6 +478,7 @@ fn place_inlined_mono_items<'tcx>( return mono_item_placements; fn follow_inlining<'tcx>( + tcx: TyCtxt<'tcx>, mono_item: MonoItem<'tcx>, inlining_map: &InliningMap<'tcx>, visited: &mut FxHashSet<MonoItem<'tcx>>, @@ -486,8 +487,8 @@ fn place_inlined_mono_items<'tcx>( return; } - inlining_map.with_inlining_candidates(mono_item, |target| { - follow_inlining(target, inlining_map, visited); + inlining_map.with_inlining_candidates(tcx, mono_item, |target| { + follow_inlining(tcx, target, inlining_map, visited); }); } } From 2eeb7693c5cb2e324458094bc8e923489a318953 Mon Sep 17 00:00:00 2001 From: anna-singleton <annabeths111@gmail.com> Date: Wed, 31 May 2023 15:51:28 +0100 Subject: [PATCH 07/11] remove reference to Into in ? operator core/std docs, fix 111655 --- library/core/src/convert/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/core/src/convert/mod.rs b/library/core/src/convert/mod.rs index 38a6d1ccdb5..799085e9a83 100644 --- a/library/core/src/convert/mod.rs +++ b/library/core/src/convert/mod.rs @@ -495,8 +495,7 @@ pub trait Into<T>: Sized { /// By converting underlying error types to our own custom error type that encapsulates the /// underlying error type, we can return a single error type without losing information on the /// underlying cause. The '?' operator automatically converts the underlying error type to our -/// custom error type by calling `Into<CliError>::into` which is automatically provided when -/// implementing `From`. The compiler then infers which implementation of `Into` should be used. +/// custom error type with `From::from`. /// /// ``` /// use std::fs; From 0a51ab93cf8a8c11f28cca7a5912a5af23e98a89 Mon Sep 17 00:00:00 2001 From: Michael Goulet <michael@errs.io> Date: Sat, 27 May 2023 18:16:55 +0000 Subject: [PATCH 08/11] Don't suggest break through nested items --- .../src/fn_ctxt/suggestions.rs | 10 +++- .../ui/loops/dont-suggest-break-thru-item.rs | 55 +++++++++++++++++++ .../loops/dont-suggest-break-thru-item.stderr | 55 +++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 tests/ui/loops/dont-suggest-break-thru-item.rs create mode 100644 tests/ui/loops/dont-suggest-break-thru-item.stderr diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index c4add4dbdfb..e19b0664461 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -874,7 +874,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let found = self.resolve_vars_with_obligations(found); let in_loop = self.is_loop(id) - || self.tcx.hir().parent_iter(id).any(|(parent_id, _)| self.is_loop(parent_id)); + || self + .tcx + .hir() + .parent_iter(id) + .take_while(|(_, node)| { + // look at parents until we find the first body owner + node.body_id().is_none() + }) + .any(|(parent_id, _)| self.is_loop(parent_id)); let in_local_statement = self.is_local_statement(id) || self diff --git a/tests/ui/loops/dont-suggest-break-thru-item.rs b/tests/ui/loops/dont-suggest-break-thru-item.rs new file mode 100644 index 00000000000..b46ba89e81d --- /dev/null +++ b/tests/ui/loops/dont-suggest-break-thru-item.rs @@ -0,0 +1,55 @@ +// edition:2021 + +#![feature(inline_const)] + +fn closure() { + loop { + let closure = || { + if true { + Err(1) + //~^ ERROR mismatched types + } + + Ok(()) + }; + } +} + +fn async_block() { + loop { + let fut = async { + if true { + Err(1) + //~^ ERROR mismatched types + } + + Ok(()) + }; + } +} + +fn fn_item() { + let _ = loop { + fn foo() -> Result<(), ()> { + if true { + Err(1) + //~^ ERROR mismatched types + } + Err(()) + } + }; +} + +fn const_block() { + let _ = loop { + const { + if true { + Err(1) + //~^ ERROR mismatched types + } + Err(()) + }; + }; +} + +fn main() {} diff --git a/tests/ui/loops/dont-suggest-break-thru-item.stderr b/tests/ui/loops/dont-suggest-break-thru-item.stderr new file mode 100644 index 00000000000..4fce4715119 --- /dev/null +++ b/tests/ui/loops/dont-suggest-break-thru-item.stderr @@ -0,0 +1,55 @@ +error[E0308]: mismatched types + --> $DIR/dont-suggest-break-thru-item.rs:9:17 + | +LL | / if true { +LL | | Err(1) + | | ^^^^^^ expected `()`, found `Result<_, {integer}>` +LL | | +LL | | } + | |_____________- expected this to be `()` + | + = note: expected unit type `()` + found enum `Result<_, {integer}>` + +error[E0308]: mismatched types + --> $DIR/dont-suggest-break-thru-item.rs:22:17 + | +LL | / if true { +LL | | Err(1) + | | ^^^^^^ expected `()`, found `Result<_, {integer}>` +LL | | +LL | | } + | |_____________- expected this to be `()` + | + = note: expected unit type `()` + found enum `Result<_, {integer}>` + +error[E0308]: mismatched types + --> $DIR/dont-suggest-break-thru-item.rs:35:17 + | +LL | / if true { +LL | | Err(1) + | | ^^^^^^ expected `()`, found `Result<_, {integer}>` +LL | | +LL | | } + | |_____________- expected this to be `()` + | + = note: expected unit type `()` + found enum `Result<_, {integer}>` + +error[E0308]: mismatched types + --> $DIR/dont-suggest-break-thru-item.rs:47:17 + | +LL | / if true { +LL | | Err(1) + | | ^^^^^^ expected `()`, found `Result<_, {integer}>` +LL | | +LL | | } + | |_____________- expected this to be `()` + | + = note: expected unit type `()` + found enum `Result<_, {integer}>` + +error: aborting due to 4 previous errors + +For more information about this error, try `rustc --explain E0308`. From df1c1afdafcf8c9c46b18f274fc7f01f13ff05da Mon Sep 17 00:00:00 2001 From: Michael Goulet <michael@errs.io> Date: Wed, 31 May 2023 17:45:45 +0000 Subject: [PATCH 09/11] Check that RPITs are compatible with the opaques inferred during HIR typeck too --- .../src/collect/type_of/opaque.rs | 65 +++++++++-------- .../rustc_middle/src/ty/typeck_results.rs | 4 -- tests/ui/chalkify/bugs/async.stderr | 13 +--- tests/ui/dyn-star/param-env-infer.next.stderr | 35 ---------- tests/ui/impl-trait/auto-trait-leak.stderr | 70 ------------------- .../multiple-defining-usages-in-body.rs | 2 +- .../multiple-defining-usages-in-body.stderr | 12 ++-- 7 files changed, 46 insertions(+), 155 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs b/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs index da82ce1d523..4d96a7ff4c3 100644 --- a/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs +++ b/compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs @@ -1,3 +1,4 @@ +use rustc_errors::StashKey; use rustc_hir::def_id::LocalDefId; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{self as hir, Expr, ImplItem, Item, Node, TraitItem}; @@ -210,12 +211,40 @@ pub(super) fn find_opaque_ty_constraints_for_rpit<'tcx>( def_id: LocalDefId, owner_def_id: LocalDefId, ) -> Ty<'_> { - let concrete = tcx.mir_borrowck(owner_def_id).concrete_opaque_types.get(&def_id).copied(); + let tables = tcx.typeck(owner_def_id); - if let Some(concrete) = concrete { + // Check that all of the opaques we inferred during HIR are compatible. + // FIXME: We explicitly don't check that the types inferred during HIR + // typeck are compatible with the one that we infer during borrowck, + // because that one actually sometimes has consts evaluated eagerly so + // using strict type equality will fail. + let mut hir_opaque_ty: Option<ty::OpaqueHiddenType<'tcx>> = None; + if tables.tainted_by_errors.is_none() { + for (&opaque_type_key, &hidden_type) in &tables.concrete_opaque_types { + if opaque_type_key.def_id != def_id { + continue; + } + let concrete_type = tcx.erase_regions( + hidden_type.remap_generic_params_to_declaration_params(opaque_type_key, tcx, true), + ); + if let Some(prev) = &mut hir_opaque_ty { + if concrete_type.ty != prev.ty && !(concrete_type, prev.ty).references_error() { + prev.report_mismatch(&concrete_type, def_id, tcx).stash( + tcx.def_span(opaque_type_key.def_id), + StashKey::OpaqueHiddenTypeMismatch, + ); + } + } else { + hir_opaque_ty = Some(concrete_type); + } + } + } + + let mir_opaque_ty = tcx.mir_borrowck(owner_def_id).concrete_opaque_types.get(&def_id).copied(); + if let Some(mir_opaque_ty) = mir_opaque_ty { let scope = tcx.hir().local_def_id_to_hir_id(owner_def_id); debug!(?scope); - let mut locator = RpitConstraintChecker { def_id, tcx, found: concrete }; + let mut locator = RpitConstraintChecker { def_id, tcx, found: mir_opaque_ty }; match tcx.hir().get(scope) { Node::Item(it) => intravisit::walk_item(&mut locator, it), @@ -224,38 +253,16 @@ pub(super) fn find_opaque_ty_constraints_for_rpit<'tcx>( other => bug!("{:?} is not a valid scope for an opaque type item", other), } - concrete.ty + mir_opaque_ty.ty } else { - let tables = tcx.typeck(owner_def_id); if let Some(guar) = tables.tainted_by_errors { - // Some error in the - // owner fn prevented us from populating + // Some error in the owner fn prevented us from populating // the `concrete_opaque_types` table. tcx.ty_error(guar) } else { // Fall back to the RPIT we inferred during HIR typeck - let mut opaque_ty: Option<ty::OpaqueHiddenType<'tcx>> = None; - for (&opaque_type_key, &hidden_type) in &tables.concrete_opaque_types { - if opaque_type_key.def_id != def_id { - continue; - } - let concrete_type = - tcx.erase_regions(hidden_type.remap_generic_params_to_declaration_params( - opaque_type_key, - tcx, - true, - )); - if let Some(prev) = &mut opaque_ty { - if concrete_type.ty != prev.ty && !(concrete_type, prev.ty).references_error() { - prev.report_mismatch(&concrete_type, def_id, tcx).emit(); - } - } else { - opaque_ty = Some(concrete_type); - } - } - - if let Some(opaque_ty) = opaque_ty { - opaque_ty.ty + if let Some(hir_opaque_ty) = hir_opaque_ty { + hir_opaque_ty.ty } else { // We failed to resolve the opaque type or it // resolves to itself. We interpret this as the diff --git a/compiler/rustc_middle/src/ty/typeck_results.rs b/compiler/rustc_middle/src/ty/typeck_results.rs index 68bae5828c5..8cbffa14850 100644 --- a/compiler/rustc_middle/src/ty/typeck_results.rs +++ b/compiler/rustc_middle/src/ty/typeck_results.rs @@ -155,10 +155,6 @@ pub struct TypeckResults<'tcx> { /// We also store the type here, so that the compiler can use it as a hint /// for figuring out hidden types, even if they are only set in dead code /// (which doesn't show up in MIR). - /// - /// These types are mapped back to the opaque's identity substitutions - /// (with erased regions), which is why we don't associated substs with any - /// of these usages. pub concrete_opaque_types: FxIndexMap<ty::OpaqueTypeKey<'tcx>, ty::OpaqueHiddenType<'tcx>>, /// Tracks the minimum captures required for a closure; diff --git a/tests/ui/chalkify/bugs/async.stderr b/tests/ui/chalkify/bugs/async.stderr index 7e64e67f24c..e6d46b02706 100644 --- a/tests/ui/chalkify/bugs/async.stderr +++ b/tests/ui/chalkify/bugs/async.stderr @@ -13,16 +13,9 @@ error: internal compiler error: projection clauses should be implied from elsewh LL | async fn foo(x: u32) -> u32 { | ^^^query stack during panic: #0 [typeck] type-checking `foo` -#1 [thir_body] building THIR for `foo` -#2 [check_match] match-checking `foo` -#3 [mir_built] building MIR for `foo` -#4 [unsafety_check_result] unsafety-checking `foo` -#5 [mir_const] preparing `foo` for borrow checking -#6 [mir_promoted] promoting constants in MIR for `foo` -#7 [mir_borrowck] borrow-checking `foo` -#8 [type_of] computing type of `foo::{opaque#0}` -#9 [check_mod_item_types] checking item types in top-level module -#10 [analysis] running analysis passes on this crate +#1 [type_of] computing type of `foo::{opaque#0}` +#2 [check_mod_item_types] checking item types in top-level module +#3 [analysis] running analysis passes on this crate end of query stack error: aborting due to 2 previous errors diff --git a/tests/ui/dyn-star/param-env-infer.next.stderr b/tests/ui/dyn-star/param-env-infer.next.stderr index 64d76bb04b1..408abecc30d 100644 --- a/tests/ui/dyn-star/param-env-infer.next.stderr +++ b/tests/ui/dyn-star/param-env-infer.next.stderr @@ -13,41 +13,6 @@ error[E0391]: cycle detected when computing type of `make_dyn_star::{opaque#0}` LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -note: ...which requires borrow-checking `make_dyn_star`... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires promoting constants in MIR for `make_dyn_star`... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires preparing `make_dyn_star` for borrow checking... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires unsafety-checking `make_dyn_star`... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires building MIR for `make_dyn_star`... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires match-checking `make_dyn_star`... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires building THIR for `make_dyn_star`... - --> $DIR/param-env-infer.rs:11:1 - | -LL | fn make_dyn_star<'a, T: PointerLike + Debug + 'a>(t: T) -> impl PointerLike + Debug + 'a { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: ...which requires type-checking `make_dyn_star`... --> $DIR/param-env-infer.rs:11:1 | diff --git a/tests/ui/impl-trait/auto-trait-leak.stderr b/tests/ui/impl-trait/auto-trait-leak.stderr index aa4ee75bb75..c0c4cd5013e 100644 --- a/tests/ui/impl-trait/auto-trait-leak.stderr +++ b/tests/ui/impl-trait/auto-trait-leak.stderr @@ -4,41 +4,6 @@ error[E0391]: cycle detected when computing type of `cycle1::{opaque#0}` LL | fn cycle1() -> impl Clone { | ^^^^^^^^^^ | -note: ...which requires borrow-checking `cycle1`... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires promoting constants in MIR for `cycle1`... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires preparing `cycle1` for borrow checking... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires unsafety-checking `cycle1`... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires building MIR for `cycle1`... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires match-checking `cycle1`... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires building THIR for `cycle1`... - --> $DIR/auto-trait-leak.rs:12:1 - | -LL | fn cycle1() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ note: ...which requires type-checking `cycle1`... --> $DIR/auto-trait-leak.rs:14:5 | @@ -50,41 +15,6 @@ note: ...which requires computing type of `cycle2::{opaque#0}`... | LL | fn cycle2() -> impl Clone { | ^^^^^^^^^^ -note: ...which requires borrow-checking `cycle2`... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires promoting constants in MIR for `cycle2`... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires preparing `cycle2` for borrow checking... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires unsafety-checking `cycle2`... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires building MIR for `cycle2`... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires match-checking `cycle2`... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -note: ...which requires building THIR for `cycle2`... - --> $DIR/auto-trait-leak.rs:19:1 - | -LL | fn cycle2() -> impl Clone { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ note: ...which requires type-checking `cycle2`... --> $DIR/auto-trait-leak.rs:20:5 | diff --git a/tests/ui/impl-trait/multiple-defining-usages-in-body.rs b/tests/ui/impl-trait/multiple-defining-usages-in-body.rs index c3a6f09f86d..86661153a0d 100644 --- a/tests/ui/impl-trait/multiple-defining-usages-in-body.rs +++ b/tests/ui/impl-trait/multiple-defining-usages-in-body.rs @@ -4,9 +4,9 @@ impl Trait for () {} fn foo<T: Trait, U: Trait>() -> impl Trait { //~^ WARN function cannot return without recursing [unconditional_recursion] let a: T = foo::<T, U>(); - //~^ ERROR concrete type differs from previous defining opaque type use loop {} let _: T = foo::<U, T>(); + //~^ ERROR concrete type differs from previous defining opaque type use } fn main() {} diff --git a/tests/ui/impl-trait/multiple-defining-usages-in-body.stderr b/tests/ui/impl-trait/multiple-defining-usages-in-body.stderr index 06991749bfa..f3c090408b4 100644 --- a/tests/ui/impl-trait/multiple-defining-usages-in-body.stderr +++ b/tests/ui/impl-trait/multiple-defining-usages-in-body.stderr @@ -11,15 +11,15 @@ LL | let a: T = foo::<T, U>(); = note: `#[warn(unconditional_recursion)]` on by default error: concrete type differs from previous defining opaque type use + --> $DIR/multiple-defining-usages-in-body.rs:8:16 + | +LL | let _: T = foo::<U, T>(); + | ^^^^^^^^^^^^^ expected `T`, got `U` + | +note: previous use here --> $DIR/multiple-defining-usages-in-body.rs:6:16 | LL | let a: T = foo::<T, U>(); - | ^^^^^^^^^^^^^ expected `U`, got `T` - | -note: previous use here - --> $DIR/multiple-defining-usages-in-body.rs:9:16 - | -LL | let _: T = foo::<U, T>(); | ^^^^^^^^^^^^^ error: aborting due to previous error; 1 warning emitted From d7d497a3c11ea7dc764d0f8d58a2a15cb395f5e9 Mon Sep 17 00:00:00 2001 From: Michael Howell <michael@notriddle.com> Date: Wed, 31 May 2023 16:55:59 -0700 Subject: [PATCH 10/11] rustdoc: add jsdoc comments for complex functions --- src/librustdoc/html/static/js/main.js | 35 ++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/html/static/js/main.js b/src/librustdoc/html/static/js/main.js index 11ed2f5f901..6da51ea0a55 100644 --- a/src/librustdoc/html/static/js/main.js +++ b/src/librustdoc/html/static/js/main.js @@ -779,6 +779,13 @@ function preLoadCss(cssUrl) { }); }); + /** + * Show a tooltip immediately. + * + * @param {DOMElement} e - The tooltip's anchor point. The DOM is consulted to figure + * out what the tooltip should contain, and where it should be + * positioned. + */ function showTooltip(e) { const notable_ty = e.getAttribute("data-notable-ty"); if (!window.NOTABLE_TRAITS && notable_ty) { @@ -789,8 +796,9 @@ function preLoadCss(cssUrl) { throw new Error("showTooltip() called with notable without any notable traits!"); } } + // Make this function idempotent. If the tooltip is already shown, avoid doing extra work + // and leave it alone. if (window.CURRENT_TOOLTIP_ELEMENT && window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE === e) { - // Make this function idempotent. clearTooltipHoverTimeout(window.CURRENT_TOOLTIP_ELEMENT); return; } @@ -800,6 +808,7 @@ function preLoadCss(cssUrl) { wrapper.innerHTML = "<div class=\"content\">" + window.NOTABLE_TRAITS[notable_ty] + "</div>"; } else { + // Replace any `title` attribute with `data-title` to avoid double tooltips. if (e.getAttribute("title") !== null) { e.setAttribute("data-title", e.getAttribute("title")); e.removeAttribute("title"); @@ -859,6 +868,17 @@ function preLoadCss(cssUrl) { }; } + /** + * Show or hide the tooltip after a timeout. If a timeout was already set before this function + * was called, that timeout gets cleared. If the tooltip is already in the requested state, + * this function will still clear any pending timeout, but otherwise do nothing. + * + * @param {DOMElement} element - The tooltip's anchor point. The DOM is consulted to figure + * out what the tooltip should contain, and where it should be + * positioned. + * @param {boolean} show - If true, the tooltip will be made visible. If false, it will + * be hidden. + */ function setTooltipHoverTimeout(element, show) { clearTooltipHoverTimeout(element); if (!show && !window.CURRENT_TOOLTIP_ELEMENT) { @@ -883,6 +903,13 @@ function preLoadCss(cssUrl) { }, show ? window.RUSTDOC_TOOLTIP_HOVER_MS : window.RUSTDOC_TOOLTIP_HOVER_EXIT_MS); } + /** + * If a show/hide timeout was set by `setTooltipHoverTimeout`, cancel it. If none exists, + * do nothing. + * + * @param {DOMElement} element - The tooltip's anchor point, + * as passed to `setTooltipHoverTimeout`. + */ function clearTooltipHoverTimeout(element) { if (element.TOOLTIP_HOVER_TIMEOUT !== undefined) { removeClass(window.CURRENT_TOOLTIP_ELEMENT, "fade-out"); @@ -910,6 +937,12 @@ function preLoadCss(cssUrl) { } } + /** + * Hide the current tooltip immediately. + * + * @param {boolean} focus - If set to `true`, move keyboard focus to the tooltip anchor point. + * If set to `false`, leave keyboard focus alone. + */ function hideTooltip(focus) { if (window.CURRENT_TOOLTIP_ELEMENT) { if (window.CURRENT_TOOLTIP_ELEMENT.TOOLTIP_BASE.TOOLTIP_FORCE_VISIBLE) { From bf36193ef6f8bc31cc2ae3ffe8ec9bfe0d3d29a8 Mon Sep 17 00:00:00 2001 From: Scott McMurray <scottmcm@users.noreply.github.com> Date: Sun, 7 May 2023 03:00:41 -0700 Subject: [PATCH 11/11] Add a distinct `OperandValue::ZeroSized` variant for ZSTs These tend to have special handling in a bunch of places anyway, so the variant helps remember that. And I think it's easier to grok than non-Scalar Aggregates sometimes being `Immediates` (like I got wrong and caused 109992). As a minor bonus, it means we don't need to generate poison LLVM values for them to pass around in `OperandValue::Immediate`s. --- compiler/rustc_codegen_gcc/src/builder.rs | 2 +- compiler/rustc_codegen_gcc/src/type_of.rs | 3 +- compiler/rustc_codegen_llvm/src/builder.rs | 2 +- compiler/rustc_codegen_llvm/src/type_of.rs | 3 +- compiler/rustc_codegen_ssa/src/base.rs | 2 +- compiler/rustc_codegen_ssa/src/mir/block.rs | 13 +++- .../rustc_codegen_ssa/src/mir/debuginfo.rs | 3 + compiler/rustc_codegen_ssa/src/mir/mod.rs | 13 ++-- compiler/rustc_codegen_ssa/src/mir/operand.rs | 49 ++++++------ compiler/rustc_codegen_ssa/src/mir/rvalue.rs | 68 +++++++++-------- tests/codegen/intrinsics/transmute.rs | 75 ++++++++++++++++++- 11 files changed, 157 insertions(+), 76 deletions(-) diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index 869344ce92d..f9ea0f00456 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -758,7 +758,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { assert_eq!(place.llextra.is_some(), place.layout.is_unsized()); if place.layout.is_zst() { - return OperandRef::new_zst(self, place.layout); + return OperandRef::zero_sized(place.layout); } fn scalar_load_metadata<'a, 'gcc, 'tcx>(bx: &mut Builder<'a, 'gcc, 'tcx>, load: RValue<'gcc>, scalar: &abi::Scalar) { diff --git a/compiler/rustc_codegen_gcc/src/type_of.rs b/compiler/rustc_codegen_gcc/src/type_of.rs index 5df8c1a209d..30a3fe67b85 100644 --- a/compiler/rustc_codegen_gcc/src/type_of.rs +++ b/compiler/rustc_codegen_gcc/src/type_of.rs @@ -159,8 +159,7 @@ impl<'tcx> LayoutGccExt<'tcx> for TyAndLayout<'tcx> { fn is_gcc_immediate(&self) -> bool { match self.abi { Abi::Scalar(_) | Abi::Vector { .. } => true, - Abi::ScalarPair(..) => false, - Abi::Uninhabited | Abi::Aggregate { .. } => self.is_zst(), + Abi::ScalarPair(..) | Abi::Uninhabited | Abi::Aggregate { .. } => false, } } diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 4d0bcd53d15..5968e70b1cc 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -486,7 +486,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { assert_eq!(place.llextra.is_some(), place.layout.is_unsized()); if place.layout.is_zst() { - return OperandRef::new_zst(self, place.layout); + return OperandRef::zero_sized(place.layout); } #[instrument(level = "trace", skip(bx))] diff --git a/compiler/rustc_codegen_llvm/src/type_of.rs b/compiler/rustc_codegen_llvm/src/type_of.rs index e264ce78f0d..a493c9c0548 100644 --- a/compiler/rustc_codegen_llvm/src/type_of.rs +++ b/compiler/rustc_codegen_llvm/src/type_of.rs @@ -198,8 +198,7 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> { fn is_llvm_immediate(&self) -> bool { match self.abi { Abi::Scalar(_) | Abi::Vector { .. } => true, - Abi::ScalarPair(..) => false, - Abi::Uninhabited | Abi::Aggregate { .. } => self.is_zst(), + Abi::ScalarPair(..) | Abi::Uninhabited | Abi::Aggregate { .. } => false, } } diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 15c7847155d..242d209b684 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -295,7 +295,7 @@ pub fn coerce_unsized_into<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let (base, info) = match bx.load_operand(src).val { OperandValue::Pair(base, info) => unsize_ptr(bx, base, src_ty, dst_ty, Some(info)), OperandValue::Immediate(base) => unsize_ptr(bx, base, src_ty, dst_ty, None), - OperandValue::Ref(..) => bug!(), + OperandValue::Ref(..) | OperandValue::ZeroSized => bug!(), }; OperandValue::Pair(base, info).store(bx, dst); } diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 3f0b64b1103..e0cb26d3ba8 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -1,5 +1,5 @@ use super::operand::OperandRef; -use super::operand::OperandValue::{Immediate, Pair, Ref}; +use super::operand::OperandValue::{Immediate, Pair, Ref, ZeroSized}; use super::place::PlaceRef; use super::{CachedLlbb, FunctionCx, LocalRef}; @@ -427,6 +427,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { assert_eq!(align, op.layout.align.abi, "return place is unaligned!"); llval } + ZeroSized => bug!("ZST return value shouldn't be in PassMode::Cast"), }; let ty = bx.cast_backend_type(cast_ty); let addr = bx.pointercast(llslot, bx.type_ptr_to(ty)); @@ -1386,6 +1387,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { (llval, align, true) } } + ZeroSized => match arg.mode { + PassMode::Indirect { .. } => { + // Though `extern "Rust"` doesn't pass ZSTs, some ABIs pass + // a pointer for `repr(C)` structs even when empty, so get + // one from an `alloca` (which can be left uninitialized). + let scratch = PlaceRef::alloca(bx, arg.layout); + (scratch.llval, scratch.align, true) + } + _ => bug!("ZST {op:?} wasn't ignored, but was passed with abi {arg:?}"), + }, }; if by_ref && !arg.is_indirect() { diff --git a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs index bba2800fb05..4f79c6a3d82 100644 --- a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs @@ -352,6 +352,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { bx.set_var_name(a, &(name.clone() + ".0")); bx.set_var_name(b, &(name.clone() + ".1")); } + OperandValue::ZeroSized => { + // These never have a value to talk about + } }, LocalRef::PendingOperand => {} } diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs index 00414003202..2809ec2deb5 100644 --- a/compiler/rustc_codegen_ssa/src/mir/mod.rs +++ b/compiler/rustc_codegen_ssa/src/mir/mod.rs @@ -129,16 +129,13 @@ enum LocalRef<'tcx, V> { PendingOperand, } -impl<'a, 'tcx, V: CodegenObject> LocalRef<'tcx, V> { - fn new_operand<Bx: BuilderMethods<'a, 'tcx, Value = V>>( - bx: &mut Bx, - layout: TyAndLayout<'tcx>, - ) -> LocalRef<'tcx, V> { +impl<'tcx, V: CodegenObject> LocalRef<'tcx, V> { + fn new_operand(layout: TyAndLayout<'tcx>) -> LocalRef<'tcx, V> { if layout.is_zst() { // Zero-size temporaries aren't always initialized, which // doesn't matter because they don't contain data, but // we need something in the operand. - LocalRef::Operand(OperandRef::new_zst(bx, layout)) + LocalRef::Operand(OperandRef::zero_sized(layout)) } else { LocalRef::PendingOperand } @@ -249,7 +246,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( } } else { debug!("alloc: {:?} -> operand", local); - LocalRef::new_operand(&mut start_bx, layout) + LocalRef::new_operand(layout) } }; @@ -355,7 +352,7 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let local = |op| LocalRef::Operand(op); match arg.mode { PassMode::Ignore => { - return local(OperandRef::new_zst(bx, arg.layout)); + return local(OperandRef::zero_sized(arg.layout)); } PassMode::Direct(_) => { let llarg = bx.get_param(llarg_idx); diff --git a/compiler/rustc_codegen_ssa/src/mir/operand.rs b/compiler/rustc_codegen_ssa/src/mir/operand.rs index 4000c9540ce..31c293d7c29 100644 --- a/compiler/rustc_codegen_ssa/src/mir/operand.rs +++ b/compiler/rustc_codegen_ssa/src/mir/operand.rs @@ -45,6 +45,14 @@ pub enum OperandValue<V> { /// as returned by [`LayoutTypeMethods::scalar_pair_element_backend_type`] /// with `immediate: true`. Pair(V, V), + /// A value taking no bytes, and which therefore needs no LLVM value at all. + /// + /// If you ever need a `V` to pass to something, get a fresh poison value + /// from [`ConstMethods::const_poison`]. + /// + /// An `OperandValue` *must* be this variant for any type for which + /// `is_zst` on its `Layout` returns `true`. + ZeroSized, } /// An `OperandRef` is an "SSA" reference to a Rust value, along with @@ -71,15 +79,9 @@ impl<V: CodegenObject> fmt::Debug for OperandRef<'_, V> { } impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { - pub fn new_zst<Bx: BuilderMethods<'a, 'tcx, Value = V>>( - bx: &mut Bx, - layout: TyAndLayout<'tcx>, - ) -> OperandRef<'tcx, V> { + pub fn zero_sized(layout: TyAndLayout<'tcx>) -> OperandRef<'tcx, V> { assert!(layout.is_zst()); - OperandRef { - val: OperandValue::Immediate(bx.const_poison(bx.immediate_backend_type(layout))), - layout, - } + OperandRef { val: OperandValue::ZeroSized, layout } } pub fn from_const<Bx: BuilderMethods<'a, 'tcx, Value = V>>( @@ -97,7 +99,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { let llval = bx.scalar_to_backend(x, scalar, bx.immediate_backend_type(layout)); OperandValue::Immediate(llval) } - ConstValue::ZeroSized => return OperandRef::new_zst(bx, layout), + ConstValue::ZeroSized => return OperandRef::zero_sized(layout), ConstValue::Slice { data, start, end } => { let Abi::ScalarPair(a_scalar, _) = layout.abi else { bug!("from_const: invalid ScalarPair layout: {:#?}", layout); @@ -178,7 +180,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { ); OperandRef { val: OperandValue::Pair(a_val, b_val), layout } } - _ if layout.is_zst() => OperandRef::new_zst(bx, layout), + _ if layout.is_zst() => OperandRef::zero_sized(layout), _ => { // Neither a scalar nor scalar pair. Load from a place let init = bx.const_data_from_alloc(alloc); @@ -216,6 +218,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { OperandValue::Immediate(llptr) => (llptr, None), OperandValue::Pair(llptr, llextra) => (llptr, Some(llextra)), OperandValue::Ref(..) => bug!("Deref of by-Ref operand {:?}", self), + OperandValue::ZeroSized => bug!("Deref of ZST operand {:?}", self), }; let layout = cx.layout_of(projected_ty); PlaceRef { llval: llptr, llextra, layout, align: layout.align.abi } @@ -273,9 +276,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { let mut val = match (self.val, self.layout.abi) { // If the field is ZST, it has no data. - _ if field.is_zst() => { - return OperandRef::new_zst(bx, field); - } + _ if field.is_zst() => OperandValue::ZeroSized, // Newtype of a scalar, scalar pair or vector. (OperandValue::Immediate(_) | OperandValue::Pair(..), _) @@ -306,6 +307,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { }; match (&mut val, field.abi) { + (OperandValue::ZeroSized, _) => {} ( OperandValue::Immediate(llval), Abi::Scalar(_) | Abi::ScalarPair(..) | Abi::Vector { .. }, @@ -359,8 +361,8 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { impl<'a, 'tcx, V: CodegenObject> OperandValue<V> { /// Returns an `OperandValue` that's generally UB to use in any way. /// - /// Depending on the `layout`, returns an `Immediate` or `Pair` containing - /// poison value(s), or a `Ref` containing a poison pointer. + /// Depending on the `layout`, returns `ZeroSized` for ZSTs, an `Immediate` or + /// `Pair` containing poison value(s), or a `Ref` containing a poison pointer. /// /// Supports sized types only. pub fn poison<Bx: BuilderMethods<'a, 'tcx, Value = V>>( @@ -368,7 +370,9 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> { layout: TyAndLayout<'tcx>, ) -> OperandValue<V> { assert!(layout.is_sized()); - if bx.cx().is_backend_immediate(layout) { + if layout.is_zst() { + OperandValue::ZeroSized + } else if bx.cx().is_backend_immediate(layout) { let ibty = bx.cx().immediate_backend_type(layout); OperandValue::Immediate(bx.const_poison(ibty)) } else if bx.cx().is_backend_scalar_pair(layout) { @@ -421,12 +425,11 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> { flags: MemFlags, ) { debug!("OperandRef::store: operand={:?}, dest={:?}", self, dest); - // Avoid generating stores of zero-sized values, because the only way to have a zero-sized - // value is through `undef`, and store itself is useless. - if dest.layout.is_zst() { - return; - } match self { + OperandValue::ZeroSized => { + // Avoid generating stores of zero-sized values, because the only way to have a zero-sized + // value is through `undef`/`poison`, and the store itself is useless. + } OperandValue::Ref(r, None, source_align) => { if flags.contains(MemFlags::NONTEMPORAL) { // HACK(nox): This is inefficient but there is no nontemporal memcpy. @@ -527,7 +530,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // checks in `codegen_consume` and `extract_field`. let elem = o.layout.field(bx.cx(), 0); if elem.is_zst() { - o = OperandRef::new_zst(bx, elem); + o = OperandRef::zero_sized(elem); } else { return None; } @@ -561,7 +564,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // ZSTs don't require any actual memory access. if layout.is_zst() { - return OperandRef::new_zst(bx, layout); + return OperandRef::zero_sized(layout); } if let Some(o) = self.maybe_codegen_consume_direct(bx, place_ref) { diff --git a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs index 0255b660380..5241a5aee00 100644 --- a/compiler/rustc_codegen_ssa/src/mir/rvalue.rs +++ b/compiler/rustc_codegen_ssa/src/mir/rvalue.rs @@ -70,6 +70,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { OperandValue::Ref(_, Some(_), _) => { bug!("unsized coercion on an unsized rvalue"); } + OperandValue::ZeroSized => { + bug!("unsized coercion on a ZST rvalue"); + } } } @@ -165,11 +168,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { } match src.val { - OperandValue::Ref(..) => { + OperandValue::Ref(..) | OperandValue::ZeroSized => { span_bug!( self.mir.span, "Operand path should have handled transmute \ - from `Ref` {src:?} to place {dst:?}" + from {src:?} to place {dst:?}" ); } OperandValue::Immediate(..) | OperandValue::Pair(..) => { @@ -220,17 +223,22 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let fake_place = PlaceRef::new_sized_aligned(cast_ptr, cast, align); Some(bx.load_operand(fake_place).val) } + OperandValue::ZeroSized => { + let OperandValueKind::ZeroSized = operand_kind else { + bug!("Found {operand_kind:?} for operand {operand:?}"); + }; + if let OperandValueKind::ZeroSized = cast_kind { + Some(OperandValue::ZeroSized) + } else { + None + } + } OperandValue::Immediate(imm) => { let OperandValueKind::Immediate(in_scalar) = operand_kind else { bug!("Found {operand_kind:?} for operand {operand:?}"); }; - if let OperandValueKind::Immediate(out_scalar) = cast_kind { - match (in_scalar, out_scalar) { - (ScalarOrZst::Zst, ScalarOrZst::Zst) => { - Some(OperandRef::new_zst(bx, cast).val) - } - (ScalarOrZst::Scalar(in_scalar), ScalarOrZst::Scalar(out_scalar)) - if in_scalar.size(self.cx) == out_scalar.size(self.cx) => + if let OperandValueKind::Immediate(out_scalar) = cast_kind + && in_scalar.size(self.cx) == out_scalar.size(self.cx) { let operand_bty = bx.backend_type(operand.layout); let cast_bty = bx.backend_type(cast); @@ -242,9 +250,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { out_scalar, cast_bty, ))) - } - _ => None, - } } else { None } @@ -457,6 +462,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { OperandValue::Ref(..) => { bug!("by-ref operand {:?} in `codegen_rvalue_operand`", operand); } + OperandValue::ZeroSized => { + bug!("zero-sized operand {:?} in `codegen_rvalue_operand`", operand); + } }; let (lldata, llextra) = base::unsize_ptr(bx, lldata, operand.layout.ty, cast.ty, llextra); @@ -490,6 +498,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { OperandValue::Ref(_, _, _) => todo!(), OperandValue::Immediate(v) => (v, None), OperandValue::Pair(v, l) => (v, Some(l)), + OperandValue::ZeroSized => bug!("ZST -- which is not PointerLike -- in DynStar"), }; let (lldata, llextra) = base::cast_to_dyn_star(bx, lldata, operand.layout, cast.ty, llextra); @@ -718,7 +727,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // According to `rvalue_creates_operand`, only ZST // aggregate rvalues are allowed to be operands. let ty = rvalue.ty(self.mir, self.cx.tcx()); - OperandRef::new_zst(bx, self.cx.layout_of(self.monomorphize(ty))) + OperandRef::zero_sized(self.cx.layout_of(self.monomorphize(ty))) } mir::Rvalue::ShallowInitBox(ref operand, content_ty) => { let operand = self.codegen_operand(bx, operand); @@ -936,6 +945,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // Can always load from a pointer as needed (OperandValueKind::Ref, _) => true, + // ZST-to-ZST is the easiest thing ever + (OperandValueKind::ZeroSized, OperandValueKind::ZeroSized) => true, + + // But if only one of them is a ZST the sizes can't match + (OperandValueKind::ZeroSized, _) | (_, OperandValueKind::ZeroSized) => false, + // Need to generate an `alloc` to get a pointer from an immediate (OperandValueKind::Immediate(..) | OperandValueKind::Pair(..), OperandValueKind::Ref) => false, @@ -979,12 +994,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { /// Gets which variant of [`OperandValue`] is expected for a particular type. fn value_kind(&self, layout: TyAndLayout<'tcx>) -> OperandValueKind { - if self.cx.is_backend_immediate(layout) { + if layout.is_zst() { + OperandValueKind::ZeroSized + } else if self.cx.is_backend_immediate(layout) { debug_assert!(!self.cx.is_backend_scalar_pair(layout)); OperandValueKind::Immediate(match layout.abi { - abi::Abi::Scalar(s) => ScalarOrZst::Scalar(s), - abi::Abi::Vector { element, .. } => ScalarOrZst::Scalar(element), - _ if layout.is_zst() => ScalarOrZst::Zst, + abi::Abi::Scalar(s) => s, + abi::Abi::Vector { element, .. } => element, x => span_bug!(self.mir.span, "Couldn't translate {x:?} as backend immediate"), }) } else if self.cx.is_backend_scalar_pair(layout) { @@ -1007,21 +1023,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { #[derive(Debug, Copy, Clone)] enum OperandValueKind { Ref, - Immediate(ScalarOrZst), + Immediate(abi::Scalar), Pair(abi::Scalar, abi::Scalar), -} - -#[derive(Debug, Copy, Clone)] -enum ScalarOrZst { - Zst, - Scalar(abi::Scalar), -} - -impl ScalarOrZst { - pub fn size(self, cx: &impl abi::HasDataLayout) -> abi::Size { - match self { - ScalarOrZst::Zst => abi::Size::ZERO, - ScalarOrZst::Scalar(s) => s.size(cx), - } - } + ZeroSized, } diff --git a/tests/codegen/intrinsics/transmute.rs b/tests/codegen/intrinsics/transmute.rs index 664e697c2a5..fe42494000e 100644 --- a/tests/codegen/intrinsics/transmute.rs +++ b/tests/codegen/intrinsics/transmute.rs @@ -14,10 +14,10 @@ use std::intrinsics::{transmute, transmute_unchecked}; // Some of these need custom MIR to not get removed by MIR optimizations. use std::intrinsics::mir::*; -enum Never {} +pub enum ZstNever {} #[repr(align(2))] -pub struct BigNever(Never, u16, Never); +pub struct BigNever(ZstNever, u16, ZstNever); #[repr(align(8))] pub struct Scalar64(i64); @@ -56,11 +56,13 @@ pub unsafe fn check_bigger_array(x: [u32; 3]) -> [u32; 7] { transmute_unchecked(x) } -// CHECK-LABEL: @check_to_uninhabited( +// CHECK-LABEL: @check_to_empty_array( #[no_mangle] #[custom_mir(dialect = "runtime", phase = "optimized")] -pub unsafe fn check_to_uninhabited(x: u16) -> BigNever { +pub unsafe fn check_to_empty_array(x: [u32; 5]) -> [u32; 0] { + // CHECK-NOT: trap // CHECK: call void @llvm.trap + // CHECK-NOT: trap mir!{ { RET = CastTransmute(x); @@ -69,6 +71,37 @@ pub unsafe fn check_to_uninhabited(x: u16) -> BigNever { } } +// CHECK-LABEL: @check_from_empty_array( +#[no_mangle] +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub unsafe fn check_from_empty_array(x: [u32; 0]) -> [u32; 5] { + // CHECK-NOT: trap + // CHECK: call void @llvm.trap + // CHECK-NOT: trap + mir!{ + { + RET = CastTransmute(x); + Return() + } + } +} + +// CHECK-LABEL: @check_to_uninhabited( +#[no_mangle] +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub unsafe fn check_to_uninhabited(x: u16) { + // CHECK-NOT: trap + // CHECK: call void @llvm.trap + // CHECK-NOT: trap + mir!{ + let temp: BigNever; + { + temp = CastTransmute(x); + Return() + } + } +} + // CHECK-LABEL: @check_from_uninhabited( #[no_mangle] #[custom_mir(dialect = "runtime", phase = "optimized")] @@ -366,6 +399,40 @@ pub unsafe fn check_issue_109992(x: ()) -> [(); 1] { } } +// CHECK-LABEL: @check_unit_to_never( +#[no_mangle] +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub unsafe fn check_unit_to_never(x: ()) { + // This uses custom MIR to avoid MIR optimizations having removed ZST ops. + + // CHECK-NOT: trap + // CHECK: call void @llvm.trap + // CHECK-NOT: trap + mir!{ + let temp: ZstNever; + { + temp = CastTransmute(x); + Return() + } + } +} + +// CHECK-LABEL: @check_unit_from_never( +#[no_mangle] +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub unsafe fn check_unit_from_never(x: ZstNever) -> () { + // This uses custom MIR to avoid MIR optimizations having removed ZST ops. + + // CHECK: start + // CHECK-NEXT: ret void + mir!{ + { + RET = CastTransmute(x); + Return() + } + } +} + // CHECK-LABEL: @check_maybe_uninit_pair(i16 %x.0, i64 %x.1) #[no_mangle] pub unsafe fn check_maybe_uninit_pair(