From b4b991e66f0a1fd0e517ed7f038129350a7ad6ce Mon Sep 17 00:00:00 2001 From: surechen Date: Tue, 23 Jul 2024 10:24:45 +0800 Subject: [PATCH 01/11] Suggest adding Result return type for associated method in E0277. For following: ```rust struct A; impl A { fn test4(&self) { let mut _file = File::create("foo.txt")?; //~^ ERROR the `?` operator can only be used in a method } ``` Suggest: ```rust impl A { fn test4(&self) -> Result<(), Box> { let mut _file = File::create("foo.txt")?; //~^ ERROR the `?` operator can only be used in a method Ok(()) } } ``` For #125997 --- .../src/error_reporting/traits/suggestions.rs | 41 +++++++++++++++--- ...turn-from-residual-sugg-issue-125997.fixed | 19 ++++++++ .../return-from-residual-sugg-issue-125997.rs | 15 +++++++ ...urn-from-residual-sugg-issue-125997.stderr | 43 ++++++++++++++++++- 4 files changed, 111 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs index 885216e62165e..9b949323a360a 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs @@ -4612,6 +4612,9 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { }) } + // For E0277 when use `?` operator, suggest adding + // a suitable return type in `FnSig`, and a default + // return value at the end of the function's body. pub(super) fn suggest_add_result_as_return_type( &self, obligation: &PredicateObligation<'tcx>, @@ -4622,19 +4625,47 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { return; } + // Only suggest for local function and associated method, + // because this suggest adding both return type in + // the `FnSig` and a default return value in the body, so it + // is not suitable for foreign function without a local body, + // and neighter for trait method which may be also implemented + // in other place, so shouldn't change it's FnSig. + fn choose_suggest_items<'tcx, 'hir>( + tcx: TyCtxt<'tcx>, + node: hir::Node<'hir>, + ) -> Option<(&'hir hir::FnDecl<'hir>, hir::BodyId)> { + match node { + hir::Node::Item(item) if let hir::ItemKind::Fn(sig, _, body_id) = item.kind => { + Some((sig.decl, body_id)) + } + hir::Node::ImplItem(item) + if let hir::ImplItemKind::Fn(sig, body_id) = item.kind => + { + let parent = tcx.parent_hir_node(item.hir_id()); + if let hir::Node::Item(item) = parent + && let hir::ItemKind::Impl(imp) = item.kind + && imp.of_trait.is_none() + { + return Some((sig.decl, body_id)); + } + None + } + _ => None, + } + } + let node = self.tcx.hir_node_by_def_id(obligation.cause.body_id); - if let hir::Node::Item(item) = node - && let hir::ItemKind::Fn(sig, _, body_id) = item.kind - && let hir::FnRetTy::DefaultReturn(ret_span) = sig.decl.output + if let Some((fn_decl, body_id)) = choose_suggest_items(self.tcx, node) + && let hir::FnRetTy::DefaultReturn(ret_span) = fn_decl.output && self.tcx.is_diagnostic_item(sym::FromResidual, trait_pred.def_id()) && trait_pred.skip_binder().trait_ref.args.type_at(0).is_unit() && let ty::Adt(def, _) = trait_pred.skip_binder().trait_ref.args.type_at(1).kind() && self.tcx.is_diagnostic_item(sym::Result, def.did()) { - let body = self.tcx.hir().body(body_id); let mut sugg_spans = vec![(ret_span, " -> Result<(), Box>".to_string())]; - + let body = self.tcx.hir().body(body_id); if let hir::ExprKind::Block(b, _) = body.value.kind && b.expr.is_none() { diff --git a/tests/ui/return/return-from-residual-sugg-issue-125997.fixed b/tests/ui/return/return-from-residual-sugg-issue-125997.fixed index b2eca69aeb902..a5a133998259b 100644 --- a/tests/ui/return/return-from-residual-sugg-issue-125997.fixed +++ b/tests/ui/return/return-from-residual-sugg-issue-125997.fixed @@ -33,6 +33,25 @@ macro_rules! mac { }; } +struct A; + +impl A { + fn test4(&self) -> Result<(), Box> { + let mut _file = File::create("foo.txt")?; + //~^ ERROR the `?` operator can only be used in a method + + Ok(()) +} + + fn test5(&self) -> Result<(), Box> { + let mut _file = File::create("foo.txt")?; + //~^ ERROR the `?` operator can only be used in a method + println!(); + + Ok(()) +} +} + fn main() -> Result<(), Box> { let mut _file = File::create("foo.txt")?; //~^ ERROR the `?` operator can only be used in a function diff --git a/tests/ui/return/return-from-residual-sugg-issue-125997.rs b/tests/ui/return/return-from-residual-sugg-issue-125997.rs index dd8550a388b86..30ca27eae45ef 100644 --- a/tests/ui/return/return-from-residual-sugg-issue-125997.rs +++ b/tests/ui/return/return-from-residual-sugg-issue-125997.rs @@ -27,6 +27,21 @@ macro_rules! mac { }; } +struct A; + +impl A { + fn test4(&self) { + let mut _file = File::create("foo.txt")?; + //~^ ERROR the `?` operator can only be used in a method + } + + fn test5(&self) { + let mut _file = File::create("foo.txt")?; + //~^ ERROR the `?` operator can only be used in a method + println!(); + } +} + fn main() { let mut _file = File::create("foo.txt")?; //~^ ERROR the `?` operator can only be used in a function diff --git a/tests/ui/return/return-from-residual-sugg-issue-125997.stderr b/tests/ui/return/return-from-residual-sugg-issue-125997.stderr index ef938f0213dfb..a59f38c2ec644 100644 --- a/tests/ui/return/return-from-residual-sugg-issue-125997.stderr +++ b/tests/ui/return/return-from-residual-sugg-issue-125997.stderr @@ -37,8 +37,47 @@ LL + Ok(()) LL + } | +error[E0277]: the `?` operator can only be used in a method that returns `Result` or `Option` (or another type that implements `FromResidual`) + --> $DIR/return-from-residual-sugg-issue-125997.rs:34:48 + | +LL | fn test4(&self) { + | --------------- this function should return `Result` or `Option` to accept `?` +LL | let mut _file = File::create("foo.txt")?; + | ^ cannot use the `?` operator in a method that returns `()` + | + = help: the trait `FromResidual>` is not implemented for `()` +help: consider adding return type + | +LL ~ fn test4(&self) -> Result<(), Box> { +LL | let mut _file = File::create("foo.txt")?; +LL | +LL ~ +LL + Ok(()) +LL + } + | + +error[E0277]: the `?` operator can only be used in a method that returns `Result` or `Option` (or another type that implements `FromResidual`) + --> $DIR/return-from-residual-sugg-issue-125997.rs:39:48 + | +LL | fn test5(&self) { + | --------------- this function should return `Result` or `Option` to accept `?` +LL | let mut _file = File::create("foo.txt")?; + | ^ cannot use the `?` operator in a method that returns `()` + | + = help: the trait `FromResidual>` is not implemented for `()` +help: consider adding return type + | +LL ~ fn test5(&self) -> Result<(), Box> { +LL | let mut _file = File::create("foo.txt")?; +LL | +LL | println!(); +LL ~ +LL + Ok(()) +LL + } + | + error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`) - --> $DIR/return-from-residual-sugg-issue-125997.rs:31:44 + --> $DIR/return-from-residual-sugg-issue-125997.rs:46:44 | LL | fn main() { | --------- this function should return `Result` or `Option` to accept `?` @@ -81,6 +120,6 @@ LL + Ok(()) LL + } | -error: aborting due to 4 previous errors +error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0277`. From d5a7c459662a4868f4a38cbafb72dce7885e027d Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Fri, 9 Aug 2024 14:26:18 -0400 Subject: [PATCH 02/11] doc: std::env::var: Returns None for names with '=' or NUL byte The documentation incorrectly stated that std::env::var could return an error for variable names containing '=' or the NUL byte. Copy the correct documentation from var_os. var_os was fixed in Commit 8a7a665, Pull Request #109894, which closed Issue #109893. This documentation was incorrectly added in commit f2c0f292, which replaced a panic in var_os by returning None, but documented the change as "May error if ...". Reference the specific error values and link to them. --- library/std/src/env.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index 50ae83090c7e1..215b7b03f04c3 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -198,13 +198,16 @@ impl fmt::Debug for VarsOs { /// /// # Errors /// -/// This function will return an error if the environment variable isn't set. +/// This function returns [`VarError::NotPresent`] if the environment variable +/// isn't set. /// -/// This function may return an error if the environment variable's name contains -/// the equal sign character (`=`) or the NUL character. +/// This function may return [`VarError::NotPresent`] if the +/// environment variable's name contains the equal sign character (`=`) or the +/// NUL character. /// -/// This function will return an error if the environment variable's value is -/// not valid Unicode. If this is not desired, consider using [`var_os`]. +/// This function will return [`VarError::NotUnicode`] if the environment +/// variable's value is not valid Unicode. If this is not desired, consider +/// using [`var_os`]. /// /// # Examples /// From 9a9cf2fd4ce7adee14a90c3af2623902f29486fb Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Sat, 17 Aug 2024 12:29:28 +0000 Subject: [PATCH 03/11] Fix bootstrap test `detect_src_and_out` on Windows --- src/bootstrap/src/core/config/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index 378d069672f5d..219c5a6ec914e 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -96,8 +96,8 @@ fn detect_src_and_out() { test(parse(""), None); { - let build_dir = if cfg!(windows) { Some("C:\\tmp") } else { Some("/tmp") }; - test(parse("build.build-dir = \"/tmp\""), build_dir); + let build_dir = if cfg!(windows) { "C:\\tmp" } else { "/tmp" }; + test(parse(&format!("build.build-dir = '{build_dir}'")), Some(build_dir)); } } From 1687c55168f3837506afcd2240a8a0b6eadcc1eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 17 Aug 2024 17:06:26 +0800 Subject: [PATCH 04/11] bootstrap: fix clean's `remove_dir_all` implementation ... by using `std::fs::remove_dir_all`, which handles a bunch of edge cases including read-only files and symlinks which are extremely tricky on Windows. --- src/bootstrap/src/core/build_steps/clean.rs | 93 ++++----------------- 1 file changed, 15 insertions(+), 78 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/clean.rs b/src/bootstrap/src/core/build_steps/clean.rs index f608e5d715e49..bcbe490c36a0f 100644 --- a/src/bootstrap/src/core/build_steps/clean.rs +++ b/src/bootstrap/src/core/build_steps/clean.rs @@ -6,7 +6,6 @@ //! directory unless the `--all` flag is present. use std::fs; -use std::io::{self, ErrorKind}; use std::path::Path; use crate::core::builder::{crate_description, Builder, RunConfig, ShouldRun, Step}; @@ -101,11 +100,11 @@ fn clean(build: &Build, all: bool, stage: Option) { return; } - rm_rf("tmp".as_ref()); + remove_dir_recursive("tmp"); // Clean the entire build directory if all { - rm_rf(&build.out); + remove_dir_recursive(&build.out); return; } @@ -136,17 +135,17 @@ fn clean_specific_stage(build: &Build, stage: u32) { } let path = t!(entry.path().canonicalize()); - rm_rf(&path); + remove_dir_recursive(&path); } } } fn clean_default(build: &Build) { - rm_rf(&build.out.join("tmp")); - rm_rf(&build.out.join("dist")); - rm_rf(&build.out.join("bootstrap").join(".last-warned-change-id")); - rm_rf(&build.out.join("bootstrap-shims-dump")); - rm_rf(&build.out.join("rustfmt.stamp")); + remove_dir_recursive(build.out.join("tmp")); + remove_dir_recursive(build.out.join("dist")); + remove_dir_recursive(build.out.join("bootstrap").join(".last-warned-change-id")); + remove_dir_recursive(build.out.join("bootstrap-shims-dump")); + remove_dir_recursive(build.out.join("rustfmt.stamp")); let mut hosts: Vec<_> = build.hosts.iter().map(|t| build.out.join(t)).collect(); // After cross-compilation, artifacts of the host architecture (which may differ from build.host) @@ -166,78 +165,16 @@ fn clean_default(build: &Build) { continue; } let path = t!(entry.path().canonicalize()); - rm_rf(&path); + remove_dir_recursive(&path); } } } -fn rm_rf(path: &Path) { - match path.symlink_metadata() { - Err(e) => { - if e.kind() == ErrorKind::NotFound { - return; - } - panic!("failed to get metadata for file {}: {}", path.display(), e); - } - Ok(metadata) => { - if metadata.file_type().is_file() || metadata.file_type().is_symlink() { - do_op(path, "remove file", |p| match fs::remove_file(p) { - #[cfg(windows)] - Err(e) - if e.kind() == std::io::ErrorKind::PermissionDenied - && p.file_name().and_then(std::ffi::OsStr::to_str) - == Some("bootstrap.exe") => - { - eprintln!("WARNING: failed to delete '{}'.", p.display()); - Ok(()) - } - r => r, - }); - - return; - } - - for file in t!(fs::read_dir(path)) { - rm_rf(&t!(file).path()); - } - - do_op(path, "remove dir", |p| match fs::remove_dir(p) { - // Check for dir not empty on Windows - // FIXME: Once `ErrorKind::DirectoryNotEmpty` is stabilized, - // match on `e.kind()` instead. - #[cfg(windows)] - Err(e) if e.raw_os_error() == Some(145) => Ok(()), - r => r, - }); - } - }; -} - -fn do_op(path: &Path, desc: &str, mut f: F) -where - F: FnMut(&Path) -> io::Result<()>, -{ - match f(path) { - Ok(()) => {} - // On windows we can't remove a readonly file, and git will often clone files as readonly. - // As a result, we have some special logic to remove readonly files on windows. - // This is also the reason that we can't use things like fs::remove_dir_all(). - #[cfg(windows)] - Err(ref e) if e.kind() == ErrorKind::PermissionDenied => { - let m = t!(path.symlink_metadata()); - let mut p = m.permissions(); - p.set_readonly(false); - t!(fs::set_permissions(path, p)); - f(path).unwrap_or_else(|e| { - // Delete symlinked directories on Windows - if m.file_type().is_symlink() && path.is_dir() && fs::remove_dir(path).is_ok() { - return; - } - panic!("failed to {} {}: {}", desc, path.display(), e); - }); - } - Err(e) => { - panic!("failed to {} {}: {}", desc, path.display(), e); - } +/// Wrapper for [`std::fs::remove_dir_all`] that panics on failure and prints the `path` we failed +/// on. +fn remove_dir_recursive>(path: P) { + let path = path.as_ref(); + if let Err(e) = fs::remove_dir_all(path) { + panic!("failed to `remove_dir_all` at `{}`: {e}", path.display()); } } From c7832b8d05d50478836532688ac90eb1d1ad9f23 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sun, 18 Aug 2024 11:34:42 +0300 Subject: [PATCH 05/11] move `Build::update_submodule` to `Config::update_submodule` During config parsing, some bootstrap logic (e.g., `download-ci-llvm`) checks certain sources and acts based on their state. This means that if path is a git submodule, bootstrap needs to update it before checking its state. Otherwise it may make incorrect assumptions by relying on outdated sources. To enable submodule updates during config parsing, we need to move the `update_submodule` function from the `Build` to `Config` instance, so we can access to it during the parsing process. Signed-off-by: onur-ozkan --- src/bootstrap/src/core/build_steps/llvm.rs | 2 +- src/bootstrap/src/core/config/config.rs | 119 ++++++++++++++++++++- src/bootstrap/src/lib.rs | 117 +------------------- 3 files changed, 122 insertions(+), 116 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index c5a1ab788016a..e1eea31b3bbf3 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -110,7 +110,7 @@ pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> L // Initialize the llvm submodule if not initialized already. // If submodules are disabled, this does nothing. - builder.update_submodule("src/llvm-project"); + builder.config.update_submodule("src/llvm-project"); let root = "src/llvm-project/llvm"; let out_dir = builder.llvm_out(target); diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index bdd9fd755aa26..ad67a1d4310fd 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -14,7 +14,7 @@ use std::sync::OnceLock; use std::{cmp, env, fs}; use build_helper::exit; -use build_helper::git::GitConfig; +use build_helper::git::{output_result, GitConfig}; use serde::{Deserialize, Deserializer}; use serde_derive::Deserialize; @@ -2509,6 +2509,123 @@ impl Config { } } + /// Given a path to the directory of a submodule, update it. + /// + /// `relative_path` should be relative to the root of the git repository, not an absolute path. + /// + /// This *does not* update the submodule if `config.toml` explicitly says + /// not to, or if we're not in a git repository (like a plain source + /// tarball). Typically [`Build::require_submodule`] should be + /// used instead to provide a nice error to the user if the submodule is + /// missing. + pub(crate) fn update_submodule(&self, relative_path: &str) { + if !self.submodules() { + return; + } + + let absolute_path = self.src.join(relative_path); + + // NOTE: The check for the empty directory is here because when running x.py the first time, + // the submodule won't be checked out. Check it out now so we can build it. + if !GitInfo::new(false, &absolute_path).is_managed_git_subrepository() + && !helpers::dir_is_empty(&absolute_path) + { + return; + } + + // Submodule updating actually happens during in the dry run mode. We need to make sure that + // all the git commands below are actually executed, because some follow-up code + // in bootstrap might depend on the submodules being checked out. Furthermore, not all + // the command executions below work with an empty output (produced during dry run). + // Therefore, all commands below are marked with `run_always()`, so that they also run in + // dry run mode. + let submodule_git = || { + let mut cmd = helpers::git(Some(&absolute_path)); + cmd.run_always(); + cmd + }; + + // Determine commit checked out in submodule. + let checked_out_hash = output(submodule_git().args(["rev-parse", "HEAD"]).as_command_mut()); + let checked_out_hash = checked_out_hash.trim_end(); + // Determine commit that the submodule *should* have. + let recorded = output( + helpers::git(Some(&self.src)) + .run_always() + .args(["ls-tree", "HEAD"]) + .arg(relative_path) + .as_command_mut(), + ); + + let actual_hash = recorded + .split_whitespace() + .nth(2) + .unwrap_or_else(|| panic!("unexpected output `{}`", recorded)); + + if actual_hash == checked_out_hash { + // already checked out + return; + } + + println!("Updating submodule {relative_path}"); + self.check_run( + helpers::git(Some(&self.src)) + .run_always() + .args(["submodule", "-q", "sync"]) + .arg(relative_path), + ); + + // Try passing `--progress` to start, then run git again without if that fails. + let update = |progress: bool| { + // Git is buggy and will try to fetch submodules from the tracking branch for *this* repository, + // even though that has no relation to the upstream for the submodule. + let current_branch = output_result( + helpers::git(Some(&self.src)) + .allow_failure() + .run_always() + .args(["symbolic-ref", "--short", "HEAD"]) + .as_command_mut(), + ) + .map(|b| b.trim().to_owned()); + + let mut git = helpers::git(Some(&self.src)).allow_failure(); + git.run_always(); + if let Ok(branch) = current_branch { + // If there is a tag named after the current branch, git will try to disambiguate by prepending `heads/` to the branch name. + // This syntax isn't accepted by `branch.{branch}`. Strip it. + let branch = branch.strip_prefix("heads/").unwrap_or(&branch); + git.arg("-c").arg(format!("branch.{branch}.remote=origin")); + } + git.args(["submodule", "update", "--init", "--recursive", "--depth=1"]); + if progress { + git.arg("--progress"); + } + git.arg(relative_path); + git + }; + if !self.check_run(&mut update(true)) { + self.check_run(&mut update(false)); + } + + // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error). + // diff-index reports the modifications through the exit status + let has_local_modifications = !self.check_run(submodule_git().allow_failure().args([ + "diff-index", + "--quiet", + "HEAD", + ])); + if has_local_modifications { + self.check_run(submodule_git().args(["stash", "push"])); + } + + self.check_run(submodule_git().args(["reset", "-q", "--hard"])); + self.check_run(submodule_git().args(["clean", "-qdfx"])); + + if has_local_modifications { + self.check_run(submodule_git().args(["stash", "pop"])); + } + } + #[cfg(feature = "bootstrap-self-test")] pub fn check_stage0_version(&self, _program_path: &Path, _component_name: &'static str) {} diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 784519a20a2d8..268392c5fb118 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -473,117 +473,6 @@ impl Build { build } - /// Given a path to the directory of a submodule, update it. - /// - /// `relative_path` should be relative to the root of the git repository, not an absolute path. - /// - /// This *does not* update the submodule if `config.toml` explicitly says - /// not to, or if we're not in a git repository (like a plain source - /// tarball). Typically [`Build::require_submodule`] should be - /// used instead to provide a nice error to the user if the submodule is - /// missing. - fn update_submodule(&self, relative_path: &str) { - if !self.config.submodules() { - return; - } - - let absolute_path = self.config.src.join(relative_path); - - // NOTE: The check for the empty directory is here because when running x.py the first time, - // the submodule won't be checked out. Check it out now so we can build it. - if !GitInfo::new(false, &absolute_path).is_managed_git_subrepository() - && !dir_is_empty(&absolute_path) - { - return; - } - - // Submodule updating actually happens during in the dry run mode. We need to make sure that - // all the git commands below are actually executed, because some follow-up code - // in bootstrap might depend on the submodules being checked out. Furthermore, not all - // the command executions below work with an empty output (produced during dry run). - // Therefore, all commands below are marked with `run_always()`, so that they also run in - // dry run mode. - let submodule_git = || { - let mut cmd = helpers::git(Some(&absolute_path)); - cmd.run_always(); - cmd - }; - - // Determine commit checked out in submodule. - let checked_out_hash = - submodule_git().args(["rev-parse", "HEAD"]).run_capture_stdout(self).stdout(); - let checked_out_hash = checked_out_hash.trim_end(); - // Determine commit that the submodule *should* have. - let recorded = helpers::git(Some(&self.src)) - .run_always() - .args(["ls-tree", "HEAD"]) - .arg(relative_path) - .run_capture_stdout(self) - .stdout(); - let actual_hash = recorded - .split_whitespace() - .nth(2) - .unwrap_or_else(|| panic!("unexpected output `{}`", recorded)); - - if actual_hash == checked_out_hash { - // already checked out - return; - } - - println!("Updating submodule {relative_path}"); - helpers::git(Some(&self.src)) - .run_always() - .args(["submodule", "-q", "sync"]) - .arg(relative_path) - .run(self); - - // Try passing `--progress` to start, then run git again without if that fails. - let update = |progress: bool| { - // Git is buggy and will try to fetch submodules from the tracking branch for *this* repository, - // even though that has no relation to the upstream for the submodule. - let current_branch = helpers::git(Some(&self.src)) - .allow_failure() - .run_always() - .args(["symbolic-ref", "--short", "HEAD"]) - .run_capture_stdout(self) - .stdout_if_ok() - .map(|s| s.trim().to_owned()); - - let mut git = helpers::git(Some(&self.src)).allow_failure(); - git.run_always(); - if let Some(branch) = current_branch { - // If there is a tag named after the current branch, git will try to disambiguate by prepending `heads/` to the branch name. - // This syntax isn't accepted by `branch.{branch}`. Strip it. - let branch = branch.strip_prefix("heads/").unwrap_or(&branch); - git.arg("-c").arg(format!("branch.{branch}.remote=origin")); - } - git.args(["submodule", "update", "--init", "--recursive", "--depth=1"]); - if progress { - git.arg("--progress"); - } - git.arg(relative_path); - git - }; - if !update(true).run(self) { - update(false).run(self); - } - - // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error). - // diff-index reports the modifications through the exit status - let has_local_modifications = - !submodule_git().allow_failure().args(["diff-index", "--quiet", "HEAD"]).run(self); - if has_local_modifications { - submodule_git().args(["stash", "push"]).run(self); - } - - submodule_git().args(["reset", "-q", "--hard"]).run(self); - submodule_git().args(["clean", "-qdfx"]).run(self); - - if has_local_modifications { - submodule_git().args(["stash", "pop"]).run(self); - } - } - /// Updates a submodule, and exits with a failure if submodule management /// is disabled and the submodule does not exist. /// @@ -598,7 +487,7 @@ impl Build { if cfg!(test) && !self.config.submodules() { return; } - self.update_submodule(submodule); + self.config.update_submodule(submodule); let absolute_path = self.config.src.join(submodule); if dir_is_empty(&absolute_path) { let maybe_enable = if !self.config.submodules() @@ -646,7 +535,7 @@ impl Build { let path = Path::new(submodule); // Don't update the submodule unless it's already been cloned. if GitInfo::new(false, path).is_managed_git_subrepository() { - self.update_submodule(submodule); + self.config.update_submodule(submodule); } } } @@ -659,7 +548,7 @@ impl Build { } if GitInfo::new(false, Path::new(submodule)).is_managed_git_subrepository() { - self.update_submodule(submodule); + self.config.update_submodule(submodule); } } From 3d0f3b209622ebbc2e4e568fa88a9c8905fcb301 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sun, 18 Aug 2024 11:40:03 +0300 Subject: [PATCH 06/11] bypass `dry_run` if the command is `run_always` Signed-off-by: onur-ozkan --- src/bootstrap/src/core/download.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/download.rs b/src/bootstrap/src/core/download.rs index ae39afa1fa270..d014239d0c570 100644 --- a/src/bootstrap/src/core/download.rs +++ b/src/bootstrap/src/core/download.rs @@ -56,7 +56,7 @@ impl Config { /// Returns false if do not execute at all, otherwise returns its /// `status.success()`. pub(crate) fn check_run(&self, cmd: &mut BootstrapCommand) -> bool { - if self.dry_run() { + if self.dry_run() && !cmd.run_always { return true; } self.verbose(|| println!("running: {cmd:?}")); From 1ca2708e77ac735adc3824501667694b4f9c1303 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Sun, 18 Aug 2024 11:42:18 +0300 Subject: [PATCH 07/11] sync llvm submodule during config parse Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 28 ++++++++++++++----------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index ad67a1d4310fd..65d9c24b6c56f 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -2730,19 +2730,23 @@ impl Config { asserts: bool, ) -> bool { let if_unchanged = || { - // Git is needed to track modifications here, but tarball source is not available. - // If not modified here or built through tarball source, we maintain consistency - // with '"if available"'. - if !self.rust_info.is_from_tarball() - && self - .last_modified_commit(&["src/llvm-project"], "download-ci-llvm", true) - .is_none() - { - // there are some untracked changes in the given paths. - false - } else { - llvm::is_ci_llvm_available(self, asserts) + if self.rust_info.is_from_tarball() { + // Git is needed for running "if-unchanged" logic. + println!( + "WARNING: 'if-unchanged' has no effect on tarball sources; ignoring `download-ci-llvm`." + ); + return false; } + + self.update_submodule("src/llvm-project"); + + // Check for untracked changes in `src/llvm-project`. + let has_changes = self + .last_modified_commit(&["src/llvm-project"], "download-ci-llvm", true) + .is_none(); + + // Return false if there are untracked changes, otherwise check if CI LLVM is available. + if has_changes { false } else { llvm::is_ci_llvm_available(self, asserts) } }; match download_ci_llvm { From b0023f5a417374aad980422b3f437f133676d4ef Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Sun, 18 Aug 2024 10:43:36 -0400 Subject: [PATCH 08/11] code review improvements --- library/std/src/env.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index 215b7b03f04c3..bbe191181c5a8 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -198,16 +198,12 @@ impl fmt::Debug for VarsOs { /// /// # Errors /// -/// This function returns [`VarError::NotPresent`] if the environment variable -/// isn't set. +/// Returns [`VarError::NotPresent`] if: +/// - The variable is not set. +/// - The variable's name contains an equal sign or NUL (`'='` or `'\0'`). /// -/// This function may return [`VarError::NotPresent`] if the -/// environment variable's name contains the equal sign character (`=`) or the -/// NUL character. -/// -/// This function will return [`VarError::NotUnicode`] if the environment -/// variable's value is not valid Unicode. If this is not desired, consider -/// using [`var_os`]. +/// Returns [`VarError::NotUnicode`] if the variable's value is not valid +/// Unicode. If this is not desired, consider using [`var_os`]. /// /// # Examples /// From dc4766536b892e91d8d499e9da7facc3006bf922 Mon Sep 17 00:00:00 2001 From: Samuel Moelius <35515885+smoelius@users.noreply.github.com> Date: Sun, 18 Aug 2024 13:14:09 -0400 Subject: [PATCH 09/11] Typo --- compiler/rustc_hir/src/hir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 6599e70346117..6c7125b75dbad 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -1395,7 +1395,7 @@ pub struct LetExpr<'hir> { pub pat: &'hir Pat<'hir>, pub ty: Option<&'hir Ty<'hir>>, pub init: &'hir Expr<'hir>, - /// `Recovered::Yes` when this let expressions is not in a syntanctically valid location. + /// `Recovered::Yes` when this let expressions is not in a syntactically valid location. /// Used to prevent building MIR in such situations. pub recovered: ast::Recovered, } From 3a9bf4551374893fdc522572ee569028186e22cc Mon Sep 17 00:00:00 2001 From: Goldstein Date: Sun, 18 Aug 2024 17:11:47 +0300 Subject: [PATCH 10/11] Check that `#[may_dangle]` is properly applied It's only valid when applied to a type or lifetime parameter in `Drop` trait implementation. --- compiler/rustc_passes/messages.ftl | 3 ++ compiler/rustc_passes/src/check_attr.rs | 23 ++++++++++- compiler/rustc_passes/src/errors.rs | 7 ++++ tests/ui/attributes/may_dangle.rs | 53 +++++++++++++++++++++++++ tests/ui/attributes/may_dangle.stderr | 50 +++++++++++++++++++++++ 5 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/ui/attributes/may_dangle.rs create mode 100644 tests/ui/attributes/may_dangle.stderr diff --git a/compiler/rustc_passes/messages.ftl b/compiler/rustc_passes/messages.ftl index 1ea4ca375f156..7d4f351560b00 100644 --- a/compiler/rustc_passes/messages.ftl +++ b/compiler/rustc_passes/messages.ftl @@ -444,6 +444,9 @@ passes_macro_export_on_decl_macro = passes_macro_use = `#[{$name}]` only has an effect on `extern crate` and modules +passes_may_dangle = + `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + passes_maybe_string_interpolation = you might have meant to use string interpolation in this string literal passes_missing_const_err = attributes `#[rustc_const_unstable]` and `#[rustc_const_stable]` require the function or method to be `const` diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index a47add929ebaa..60c8c1e7a0017 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/compiler/rustc_passes/src/check_attr.rs @@ -189,6 +189,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> { [sym::collapse_debuginfo, ..] => self.check_collapse_debuginfo(attr, span, target), [sym::must_not_suspend, ..] => self.check_must_not_suspend(attr, span, target), [sym::must_use, ..] => self.check_must_use(hir_id, attr, target), + [sym::may_dangle, ..] => self.check_may_dangle(hir_id, attr), [sym::rustc_pass_by_value, ..] => self.check_pass_by_value(attr, span, target), [sym::rustc_allow_incoherent_impl, ..] => { self.check_allow_incoherent_impl(attr, span, target) @@ -255,7 +256,6 @@ impl<'tcx> CheckAttrVisitor<'tcx> { | sym::cfg_attr // need to be fixed | sym::cfi_encoding // FIXME(cfi_encoding) - | sym::may_dangle // FIXME(dropck_eyepatch) | sym::pointee // FIXME(derive_smart_pointer) | sym::omit_gdb_pretty_printer_section // FIXME(omit_gdb_pretty_printer_section) | sym::used // handled elsewhere to restrict to static items @@ -1373,6 +1373,27 @@ impl<'tcx> CheckAttrVisitor<'tcx> { } } + /// Checks if `#[may_dangle]` is applied to a lifetime or type generic parameter in `Drop` impl. + fn check_may_dangle(&self, hir_id: HirId, attr: &Attribute) { + if let hir::Node::GenericParam(param) = self.tcx.hir_node(hir_id) + && matches!( + param.kind, + hir::GenericParamKind::Lifetime { .. } | hir::GenericParamKind::Type { .. } + ) + && matches!(param.source, hir::GenericParamSource::Generics) + && let parent_hir_id = self.tcx.parent_hir_id(hir_id) + && let hir::Node::Item(item) = self.tcx.hir_node(parent_hir_id) + && let hir::ItemKind::Impl(impl_) = item.kind + && let Some(trait_) = impl_.of_trait + && let Some(def_id) = trait_.trait_def_id() + && self.tcx.is_lang_item(def_id, hir::LangItem::Drop) + { + return; + } + + self.dcx().emit_err(errors::InvalidMayDangle { attr_span: attr.span }); + } + /// Checks if `#[cold]` is applied to a non-function. fn check_cold(&self, hir_id: HirId, attr: &Attribute, span: Span, target: Target) { match target { diff --git a/compiler/rustc_passes/src/errors.rs b/compiler/rustc_passes/src/errors.rs index 3a043e0e3c196..ee7d097e5d387 100644 --- a/compiler/rustc_passes/src/errors.rs +++ b/compiler/rustc_passes/src/errors.rs @@ -737,6 +737,13 @@ pub struct NonExportedMacroInvalidAttrs { pub attr_span: Span, } +#[derive(Diagnostic)] +#[diag(passes_may_dangle)] +pub struct InvalidMayDangle { + #[primary_span] + pub attr_span: Span, +} + #[derive(LintDiagnostic)] #[diag(passes_unused_duplicate)] pub struct UnusedDuplicate { diff --git a/tests/ui/attributes/may_dangle.rs b/tests/ui/attributes/may_dangle.rs new file mode 100644 index 0000000000000..209ba0e88ad18 --- /dev/null +++ b/tests/ui/attributes/may_dangle.rs @@ -0,0 +1,53 @@ +#![feature(dropck_eyepatch)] + +struct Implee1<'a, T, const N: usize>(&'a T); +struct Implee2<'a, T, const N: usize>(&'a T); +struct Implee3<'a, T, const N: usize>(&'a T); +trait NotDrop {} + +unsafe impl<#[may_dangle] 'a, T, const N: usize> NotDrop for Implee1<'a, T, N> {} +//~^ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl + +unsafe impl<'a, #[may_dangle] T, const N: usize> NotDrop for Implee2<'a, T, N> {} +//~^ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl + +unsafe impl<'a, T, #[may_dangle] const N: usize> Drop for Implee1<'a, T, N> { + //~^ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl + fn drop(&mut self) {} +} + +// Ok, lifetime param in a `Drop` impl. +unsafe impl<#[may_dangle] 'a, T, const N: usize> Drop for Implee2<'a, T, N> { + fn drop(&mut self) {} +} + +// Ok, type param in a `Drop` impl. +unsafe impl<'a, #[may_dangle] T, const N: usize> Drop for Implee3<'a, T, N> { + fn drop(&mut self) {} +} + +// Check that this check is not textual. +mod fake { + trait Drop { + fn drop(&mut self); + } + struct Implee(T); + + unsafe impl<#[may_dangle] T> Drop for Implee { + //~^ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl + fn drop(&mut self) {} + } +} + +#[may_dangle] //~ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl +struct Dangling; + +#[may_dangle] //~ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl +impl NotDrop for () { +} + +#[may_dangle] //~ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl +fn main() { + #[may_dangle] //~ ERROR must be applied to a lifetime or type generic parameter in `Drop` impl + let () = (); +} diff --git a/tests/ui/attributes/may_dangle.stderr b/tests/ui/attributes/may_dangle.stderr new file mode 100644 index 0000000000000..dc24f847f712f --- /dev/null +++ b/tests/ui/attributes/may_dangle.stderr @@ -0,0 +1,50 @@ +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:8:13 + | +LL | unsafe impl<#[may_dangle] 'a, T, const N: usize> NotDrop for Implee1<'a, T, N> {} + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:11:17 + | +LL | unsafe impl<'a, #[may_dangle] T, const N: usize> NotDrop for Implee2<'a, T, N> {} + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:14:20 + | +LL | unsafe impl<'a, T, #[may_dangle] const N: usize> Drop for Implee1<'a, T, N> { + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:42:1 + | +LL | #[may_dangle] + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:45:1 + | +LL | #[may_dangle] + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:49:1 + | +LL | #[may_dangle] + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:51:5 + | +LL | #[may_dangle] + | ^^^^^^^^^^^^^ + +error: `#[may_dangle]` must be applied to a lifetime or type generic parameter in `Drop` impl + --> $DIR/may_dangle.rs:36:17 + | +LL | unsafe impl<#[may_dangle] T> Drop for Implee { + | ^^^^^^^^^^^^^ + +error: aborting due to 8 previous errors + From df6cb954bb50fa8567f7198c02db90edda3cd3e4 Mon Sep 17 00:00:00 2001 From: Goldstein Date: Sun, 18 Aug 2024 17:15:29 +0300 Subject: [PATCH 11/11] Fix wording of misapplied `must_not_suspend` error --- compiler/rustc_passes/messages.ftl | 4 ++-- compiler/rustc_passes/src/check_attr.rs | 2 +- tests/ui/lint/must_not_suspend/other_items.stderr | 4 ++-- tests/ui/lint/must_not_suspend/return.stderr | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_passes/messages.ftl b/compiler/rustc_passes/messages.ftl index 7d4f351560b00..dfc726efeb998 100644 --- a/compiler/rustc_passes/messages.ftl +++ b/compiler/rustc_passes/messages.ftl @@ -478,8 +478,8 @@ passes_multiple_start_functions = .previous = previous `#[start]` function here passes_must_not_suspend = - `must_not_suspend` attribute should be applied to a struct, enum, or trait - .label = is not a struct, enum, or trait + `must_not_suspend` attribute should be applied to a struct, enum, union, or trait + .label = is not a struct, enum, union, or trait passes_must_use_async = `must_use` attribute on `async` functions applies to the anonymous `Future` returned by the function, not the value within diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index 60c8c1e7a0017..e3c2999142f3b 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/compiler/rustc_passes/src/check_attr.rs @@ -1363,7 +1363,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> { } } - /// Checks if `#[must_not_suspend]` is applied to a function. + /// Checks if `#[must_not_suspend]` is applied to a struct, enum, union, or trait. fn check_must_not_suspend(&self, attr: &Attribute, span: Span, target: Target) { match target { Target::Struct | Target::Enum | Target::Union | Target::Trait => {} diff --git a/tests/ui/lint/must_not_suspend/other_items.stderr b/tests/ui/lint/must_not_suspend/other_items.stderr index e6c36b7895109..dff5210b7e47f 100644 --- a/tests/ui/lint/must_not_suspend/other_items.stderr +++ b/tests/ui/lint/must_not_suspend/other_items.stderr @@ -1,10 +1,10 @@ -error: `must_not_suspend` attribute should be applied to a struct, enum, or trait +error: `must_not_suspend` attribute should be applied to a struct, enum, union, or trait --> $DIR/other_items.rs:5:1 | LL | #[must_not_suspend] | ^^^^^^^^^^^^^^^^^^^ LL | mod inner {} - | ------------ is not a struct, enum, or trait + | ------------ is not a struct, enum, union, or trait error: aborting due to 1 previous error diff --git a/tests/ui/lint/must_not_suspend/return.stderr b/tests/ui/lint/must_not_suspend/return.stderr index 5a73064c78776..440f816568622 100644 --- a/tests/ui/lint/must_not_suspend/return.stderr +++ b/tests/ui/lint/must_not_suspend/return.stderr @@ -1,4 +1,4 @@ -error: `must_not_suspend` attribute should be applied to a struct, enum, or trait +error: `must_not_suspend` attribute should be applied to a struct, enum, union, or trait --> $DIR/return.rs:5:1 | LL | #[must_not_suspend] @@ -6,7 +6,7 @@ LL | #[must_not_suspend] LL | / fn foo() -> i32 { LL | | 0 LL | | } - | |_- is not a struct, enum, or trait + | |_- is not a struct, enum, union, or trait error: aborting due to 1 previous error