From 713becd7daf6aa2d8b702bacf5681f237f315762 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 26 Mar 2025 18:54:35 +0100 Subject: [PATCH 01/18] refactor: Move Apple SDK names to rustc_codegen_ssa::back::apple --- compiler/rustc_codegen_ssa/messages.ftl | 2 -- compiler/rustc_codegen_ssa/src/back/apple.rs | 18 ++++++++++++ compiler/rustc_codegen_ssa/src/back/link.rs | 30 ++------------------ compiler/rustc_codegen_ssa/src/errors.rs | 7 ----- 4 files changed, 21 insertions(+), 36 deletions(-) diff --git a/compiler/rustc_codegen_ssa/messages.ftl b/compiler/rustc_codegen_ssa/messages.ftl index 95912b0160072..a33c0aa583495 100644 --- a/compiler/rustc_codegen_ssa/messages.ftl +++ b/compiler/rustc_codegen_ssa/messages.ftl @@ -391,8 +391,6 @@ codegen_ssa_unknown_atomic_ordering = unknown ordering in atomic intrinsic codegen_ssa_unknown_reuse_kind = unknown cgu-reuse-kind `{$kind}` specified -codegen_ssa_unsupported_arch = unsupported arch `{$arch}` for os `{$os}` - codegen_ssa_unsupported_instruction_set = target does not support `#[instruction_set]` codegen_ssa_unsupported_link_self_contained = option `-C link-self-contained` is not supported on this target diff --git a/compiler/rustc_codegen_ssa/src/back/apple.rs b/compiler/rustc_codegen_ssa/src/back/apple.rs index bfa7635a869f8..24995be6f9446 100644 --- a/compiler/rustc_codegen_ssa/src/back/apple.rs +++ b/compiler/rustc_codegen_ssa/src/back/apple.rs @@ -11,6 +11,24 @@ use crate::errors::AppleDeploymentTarget; #[cfg(test)] mod tests; +/// The canonical name of the desired SDK for a given target. +pub(super) fn sdk_name(target: &Target) -> &'static str { + match (&*target.os, &*target.abi) { + ("macos", "") => "MacOSX", + ("ios", "") => "iPhoneOS", + ("ios", "sim") => "iPhoneSimulator", + // Mac Catalyst uses the macOS SDK + ("ios", "macabi") => "MacOSX", + ("tvos", "") => "AppleTVOS", + ("tvos", "sim") => "AppleTVSimulator", + ("visionos", "") => "XROS", + ("visionos", "sim") => "XRSimulator", + ("watchos", "") => "WatchOS", + ("watchos", "sim") => "WatchSimulator", + (os, abi) => unreachable!("invalid os '{os}' / abi '{abi}' combination for Apple target"), + } +} + pub(super) fn macho_platform(target: &Target) -> u32 { match (&*target.os, &*target.abi) { ("macos", _) => object::macho::PLATFORM_MACOS, diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index 74597f6263d4f..776f381ded4f5 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -3201,9 +3201,7 @@ fn add_apple_link_args(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavo } fn add_apple_sdk(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavor) -> Option { - let arch = &sess.target.arch; let os = &sess.target.os; - let llvm_target = &sess.target.llvm_target; if sess.target.vendor != "apple" || !matches!(os.as_ref(), "ios" | "tvos" | "watchos" | "visionos" | "macos") || !matches!(flavor, LinkerFlavor::Darwin(..)) @@ -3215,31 +3213,9 @@ fn add_apple_sdk(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavor) -> return None; } - let sdk_name = match (arch.as_ref(), os.as_ref()) { - ("aarch64", "tvos") if llvm_target.ends_with("-simulator") => "appletvsimulator", - ("aarch64", "tvos") => "appletvos", - ("x86_64", "tvos") => "appletvsimulator", - ("arm", "ios") => "iphoneos", - ("aarch64", "ios") if llvm_target.contains("macabi") => "macosx", - ("aarch64", "ios") if llvm_target.ends_with("-simulator") => "iphonesimulator", - ("aarch64", "ios") => "iphoneos", - ("x86", "ios") => "iphonesimulator", - ("x86_64", "ios") if llvm_target.contains("macabi") => "macosx", - ("x86_64", "ios") => "iphonesimulator", - ("x86_64", "watchos") => "watchsimulator", - ("arm64_32", "watchos") => "watchos", - ("aarch64", "watchos") if llvm_target.ends_with("-simulator") => "watchsimulator", - ("aarch64", "watchos") => "watchos", - ("aarch64", "visionos") if llvm_target.ends_with("-simulator") => "xrsimulator", - ("aarch64", "visionos") => "xros", - ("arm", "watchos") => "watchos", - (_, "macos") => "macosx", - _ => { - sess.dcx().emit_err(errors::UnsupportedArch { arch, os }); - return None; - } - }; - let sdk_root = match get_apple_sdk_root(sdk_name) { + let sdk_name = apple::sdk_name(&sess.target).to_lowercase(); + + let sdk_root = match get_apple_sdk_root(&sdk_name) { Ok(s) => s, Err(e) => { sess.dcx().emit_err(e); diff --git a/compiler/rustc_codegen_ssa/src/errors.rs b/compiler/rustc_codegen_ssa/src/errors.rs index 0b7cad0c2fd8a..9c4c205dfe552 100644 --- a/compiler/rustc_codegen_ssa/src/errors.rs +++ b/compiler/rustc_codegen_ssa/src/errors.rs @@ -738,13 +738,6 @@ pub enum ExtractBundledLibsError<'a> { ExtractSection { rlib: &'a Path, error: Box }, } -#[derive(Diagnostic)] -#[diag(codegen_ssa_unsupported_arch)] -pub(crate) struct UnsupportedArch<'a> { - pub arch: &'a str, - pub os: &'a str, -} - #[derive(Diagnostic)] pub(crate) enum AppleDeploymentTarget { #[diag(codegen_ssa_apple_deployment_target_invalid)] From dffb0dbc3e2f28a82085025aec4aea6a6a28b611 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 27 Mar 2025 03:10:29 +0100 Subject: [PATCH 02/18] Document how the SDK is found if SDKROOT is not set I've intentionally used slightly vague language ("roughly"), as we don't want to guarantee the exact invocation of `xcrun`, just hint that it's close to that. --- src/doc/rustc/src/platform-support/apple-darwin.md | 3 ++- src/doc/rustc/src/platform-support/apple-ios-macabi.md | 3 ++- src/doc/rustc/src/platform-support/apple-ios.md | 3 ++- src/doc/rustc/src/platform-support/apple-tvos.md | 3 ++- src/doc/rustc/src/platform-support/apple-visionos.md | 3 ++- src/doc/rustc/src/platform-support/apple-watchos.md | 3 ++- 6 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/doc/rustc/src/platform-support/apple-darwin.md b/src/doc/rustc/src/platform-support/apple-darwin.md index dba2c4b2aaf0e..22c54d04b1eb8 100644 --- a/src/doc/rustc/src/platform-support/apple-darwin.md +++ b/src/doc/rustc/src/platform-support/apple-darwin.md @@ -70,4 +70,5 @@ the `-mmacosx-version-min=...`, `-miphoneos-version-min=...` or similar flags to disambiguate. The path to the SDK can be passed to `rustc` using the common `SDKROOT` -environment variable. +environment variable, or will be inferred when compiling on host macOS using +roughly the same logic as `xcrun --sdk macosx --show-sdk-path`. diff --git a/src/doc/rustc/src/platform-support/apple-ios-macabi.md b/src/doc/rustc/src/platform-support/apple-ios-macabi.md index a54656190d1f3..79966d908d898 100644 --- a/src/doc/rustc/src/platform-support/apple-ios-macabi.md +++ b/src/doc/rustc/src/platform-support/apple-ios-macabi.md @@ -20,7 +20,8 @@ These targets are cross-compiled, and require the corresponding macOS SDK iOS-specific headers, as provided by Xcode 11 or higher. The path to the SDK can be passed to `rustc` using the common `SDKROOT` -environment variable. +environment variable, or will be inferred when compiling on host macOS using +roughly the same logic as `xcrun --sdk macosx --show-sdk-path`. ### OS version diff --git a/src/doc/rustc/src/platform-support/apple-ios.md b/src/doc/rustc/src/platform-support/apple-ios.md index cfb458fdb737c..7f5dc361c49d7 100644 --- a/src/doc/rustc/src/platform-support/apple-ios.md +++ b/src/doc/rustc/src/platform-support/apple-ios.md @@ -26,7 +26,8 @@ These targets are cross-compiled, and require the corresponding iOS SDK ARM64 targets, Xcode 12 or higher is required. The path to the SDK can be passed to `rustc` using the common `SDKROOT` -environment variable. +environment variable, or will be inferred when compiling on host macOS using +roughly the same logic as `xcrun --sdk iphoneos --show-sdk-path`. ### OS version diff --git a/src/doc/rustc/src/platform-support/apple-tvos.md b/src/doc/rustc/src/platform-support/apple-tvos.md index 166bb1b6db293..fc46db20074f4 100644 --- a/src/doc/rustc/src/platform-support/apple-tvos.md +++ b/src/doc/rustc/src/platform-support/apple-tvos.md @@ -20,7 +20,8 @@ These targets are cross-compiled, and require the corresponding tvOS SDK ARM64 targets, Xcode 12 or higher is required. The path to the SDK can be passed to `rustc` using the common `SDKROOT` -environment variable. +environment variable, or will be inferred when compiling on host macOS using +roughly the same logic as `xcrun --sdk appletvos --show-sdk-path`. ### OS version diff --git a/src/doc/rustc/src/platform-support/apple-visionos.md b/src/doc/rustc/src/platform-support/apple-visionos.md index a7bbae168a4f6..7cf9549227d9a 100644 --- a/src/doc/rustc/src/platform-support/apple-visionos.md +++ b/src/doc/rustc/src/platform-support/apple-visionos.md @@ -18,7 +18,8 @@ These targets are cross-compiled, and require the corresponding visionOS SDK (`XROS.sdk` or `XRSimulator.sdk`), as provided by Xcode 15 or newer. The path to the SDK can be passed to `rustc` using the common `SDKROOT` -environment variable. +environment variable, or will be inferred when compiling on host macOS using +roughly the same logic as `xcrun --sdk xros --show-sdk-path`. ### OS version diff --git a/src/doc/rustc/src/platform-support/apple-watchos.md b/src/doc/rustc/src/platform-support/apple-watchos.md index 0bf8cdf361427..7b12d9ebfd4b7 100644 --- a/src/doc/rustc/src/platform-support/apple-watchos.md +++ b/src/doc/rustc/src/platform-support/apple-watchos.md @@ -24,7 +24,8 @@ These targets are cross-compiled, and require the corresponding watchOS SDK ARM64 targets, Xcode 12 or higher is required. The path to the SDK can be passed to `rustc` using the common `SDKROOT` -environment variable. +environment variable, or will be inferred when compiling on host macOS using +roughly the same logic as `xcrun --sdk watchos --show-sdk-path`. ### OS version From bd1ef0fad2e8172549a07fbf17dc1f9c4ceea078 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 27 Mar 2025 03:10:58 +0100 Subject: [PATCH 03/18] Invoke xcrun inside sess.time It can be a fairly expensive operation when the output is not cached, so it's nice to get some visibility into the runtime cost. --- compiler/rustc_codegen_ssa/src/back/link.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index 776f381ded4f5..eb6aebf23203b 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -3215,7 +3215,7 @@ fn add_apple_sdk(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavor) -> let sdk_name = apple::sdk_name(&sess.target).to_lowercase(); - let sdk_root = match get_apple_sdk_root(&sdk_name) { + let sdk_root = match sess.time("get_apple_sdk_root", || get_apple_sdk_root(&sdk_name)) { Ok(s) => s, Err(e) => { sess.dcx().emit_err(e); From 89348e51e383a6300206e3356143fce812838921 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 27 Mar 2025 03:15:58 +0100 Subject: [PATCH 04/18] Emit better error messages when invoking xcrun Also allow the SDK path to be non-UTF-8. --- compiler/rustc_codegen_ssa/messages.ftl | 19 ++- compiler/rustc_codegen_ssa/src/back/apple.rs | 136 +++++++++++++++++- .../rustc_codegen_ssa/src/back/apple/tests.rs | 68 ++++++++- compiler/rustc_codegen_ssa/src/back/link.rs | 59 +++----- compiler/rustc_codegen_ssa/src/errors.rs | 29 +++- compiler/rustc_codegen_ssa/src/lib.rs | 1 + 6 files changed, 263 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_codegen_ssa/messages.ftl b/compiler/rustc_codegen_ssa/messages.ftl index a33c0aa583495..954a601480935 100644 --- a/compiler/rustc_codegen_ssa/messages.ftl +++ b/compiler/rustc_codegen_ssa/messages.ftl @@ -10,8 +10,6 @@ codegen_ssa_apple_deployment_target_invalid = codegen_ssa_apple_deployment_target_too_low = deployment target in {$env_var} was set to {$version}, but the minimum supported by `rustc` is {$os_min} -codegen_ssa_apple_sdk_error_sdk_path = failed to get {$sdk_name} SDK path: {$error} - codegen_ssa_archive_build_failure = failed to build archive at `{$path}`: {$error} codegen_ssa_atomic_compare_exchange = Atomic compare-exchange intrinsic missing failure memory ordering @@ -400,3 +398,20 @@ codegen_ssa_use_cargo_directive = use the `cargo:rustc-link-lib` directive to sp codegen_ssa_version_script_write_failure = failed to write version script: {$error} codegen_ssa_visual_studio_not_installed = you may need to install Visual Studio build tools with the "C++ build tools" workload + +codegen_ssa_xcrun_command_line_tools_insufficient = + when compiling for iOS, tvOS, visionOS or watchOS, you need a full installation of Xcode + +codegen_ssa_xcrun_failed_invoking = invoking `{$command_formatted}` to find {$sdk_name}.sdk failed: {$error} + +codegen_ssa_xcrun_found_developer_dir = found active developer directory at "{$developer_dir}" + +# `xcrun` already outputs a message about missing Xcode installation, so we only augment it with details about env vars. +codegen_ssa_xcrun_no_developer_dir = + pass the path of an Xcode installation via the DEVELOPER_DIR environment variable, or an SDK with the SDKROOT environment variable + +codegen_ssa_xcrun_sdk_path_warning = output of `xcrun` while finding {$sdk_name}.sdk + .note = {$stderr} + +codegen_ssa_xcrun_unsuccessful = failed running `{$command_formatted}` to find {$sdk_name}.sdk + .note = {$stdout}{$stderr} diff --git a/compiler/rustc_codegen_ssa/src/back/apple.rs b/compiler/rustc_codegen_ssa/src/back/apple.rs index 24995be6f9446..2c8b0ec418dd8 100644 --- a/compiler/rustc_codegen_ssa/src/back/apple.rs +++ b/compiler/rustc_codegen_ssa/src/back/apple.rs @@ -1,12 +1,18 @@ use std::env; +use std::ffi::OsString; use std::fmt::{Display, from_fn}; use std::num::ParseIntError; +use std::path::PathBuf; +use std::process::Command; +use itertools::Itertools; use rustc_middle::middle::exported_symbols::SymbolExportKind; use rustc_session::Session; use rustc_target::spec::Target; +use tracing::debug; -use crate::errors::AppleDeploymentTarget; +use crate::errors::{AppleDeploymentTarget, XcrunError, XcrunSdkPathWarning}; +use crate::fluent_generated as fluent; #[cfg(test)] mod tests; @@ -271,3 +277,131 @@ pub(super) fn add_version_to_llvm_target( format!("{arch}-{vendor}-{os}{major}.{minor}.{patch}") } } + +pub(super) fn get_sdk_root(sess: &Session) -> Option { + let sdk_name = sdk_name(&sess.target); + + match xcrun_show_sdk_path(sdk_name, sess.verbose_internals()) { + Ok((path, stderr)) => { + // Emit extra stderr, such as if `-verbose` was passed, or if `xcrun` emitted a warning. + if !stderr.is_empty() { + sess.dcx().emit_warn(XcrunSdkPathWarning { sdk_name, stderr }); + } + Some(path) + } + Err(err) => { + let mut diag = sess.dcx().create_err(err); + + // Recognize common error cases, and give more Rust-specific error messages for those. + if let Some(developer_dir) = xcode_select_developer_dir() { + diag.arg("developer_dir", &developer_dir); + diag.note(fluent::codegen_ssa_xcrun_found_developer_dir); + if developer_dir.as_os_str().to_string_lossy().contains("CommandLineTools") { + if sdk_name != "MacOSX" { + diag.help(fluent::codegen_ssa_xcrun_command_line_tools_insufficient); + } + } + } else { + diag.help(fluent::codegen_ssa_xcrun_no_developer_dir); + } + + diag.emit(); + None + } + } +} + +/// Invoke `xcrun --sdk $sdk_name --show-sdk-path` to get the SDK path. +/// +/// The exact logic that `xcrun` uses is unspecified (see `man xcrun` for a few details), and may +/// change between macOS and Xcode versions, but it roughly boils down to finding the active +/// developer directory, and then invoking `xcodebuild -sdk $sdk_name -version` to get the SDK +/// details. +/// +/// Finding the developer directory is roughly done by looking at, in order: +/// - The `DEVELOPER_DIR` environment variable. +/// - The `/var/db/xcode_select_link` symlink (set by `xcode-select --switch`). +/// - `/Applications/Xcode.app` (hardcoded fallback path). +/// - `/Library/Developer/CommandLineTools` (hardcoded fallback path). +/// +/// Note that `xcrun` caches its result, but with a cold cache this whole operation can be quite +/// slow, especially so the first time it's run after a reboot. +fn xcrun_show_sdk_path( + sdk_name: &'static str, + verbose: bool, +) -> Result<(PathBuf, String), XcrunError> { + let mut cmd = Command::new("xcrun"); + if verbose { + cmd.arg("--verbose"); + } + // The `--sdk` parameter is the same as in xcodebuild, namely either an absolute path to an SDK, + // or the (lowercase) canonical name of an SDK. + cmd.arg("--sdk"); + cmd.arg(&sdk_name.to_lowercase()); + cmd.arg("--show-sdk-path"); + + // We do not stream stdout/stderr lines directly to the user, since whether they are warnings or + // errors depends on the status code at the end. + let output = cmd.output().map_err(|error| XcrunError::FailedInvoking { + sdk_name, + command_formatted: format!("{cmd:?}"), + error, + })?; + + // It is fine to do lossy conversion here, non-UTF-8 paths are quite rare on macOS nowadays + // (only possible with the HFS+ file system), and we only use it for error messages. + let stderr = String::from_utf8_lossy_owned(output.stderr); + if !stderr.is_empty() { + debug!(stderr, "original xcrun stderr"); + } + + // Some versions of `xcodebuild` output beefy errors when invoked via `xcrun`, + // but these are usually red herrings. + let stderr = stderr + .lines() + .filter(|line| { + !line.contains("Writing error result bundle") + && !line.contains("Requested but did not find extension point with identifier") + }) + .join("\n"); + + if output.status.success() { + Ok((stdout_to_path(output.stdout), stderr)) + } else { + // Output both stdout and stderr, since shims of `xcrun` (such as the one provided by + // nixpkgs), do not always use stderr for errors. + let stdout = String::from_utf8_lossy_owned(output.stdout).trim().to_string(); + Err(XcrunError::Unsuccessful { + sdk_name, + command_formatted: format!("{cmd:?}"), + stdout, + stderr, + }) + } +} + +/// Invoke `xcode-select --print-path`, and return the current developer directory. +/// +/// NOTE: We don't do any error handling here, this is only used as a canary in diagnostics (`xcrun` +/// will have already emitted the relevant error information). +fn xcode_select_developer_dir() -> Option { + let mut cmd = Command::new("xcode-select"); + cmd.arg("--print-path"); + let output = cmd.output().ok()?; + if !output.status.success() { + return None; + } + Some(stdout_to_path(output.stdout)) +} + +fn stdout_to_path(mut stdout: Vec) -> PathBuf { + // Remove trailing newline. + if let Some(b'\n') = stdout.last() { + let _ = stdout.pop().unwrap(); + } + #[cfg(unix)] + let path = ::from_vec(stdout); + #[cfg(not(unix))] // Unimportant, this is only used on macOS + let path = OsString::from(String::from_utf8(stdout).unwrap()); + PathBuf::from(path) +} diff --git a/compiler/rustc_codegen_ssa/src/back/apple/tests.rs b/compiler/rustc_codegen_ssa/src/back/apple/tests.rs index 7ccda5a8190c7..8df740a4bcf7d 100644 --- a/compiler/rustc_codegen_ssa/src/back/apple/tests.rs +++ b/compiler/rustc_codegen_ssa/src/back/apple/tests.rs @@ -1,4 +1,4 @@ -use super::{add_version_to_llvm_target, parse_version}; +use super::*; #[test] fn test_add_version_to_llvm_target() { @@ -19,3 +19,69 @@ fn test_parse_version() { assert_eq!(parse_version("10.12.6"), Ok((10, 12, 6))); assert_eq!(parse_version("9999.99.99"), Ok((9999, 99, 99))); } + +#[test] +#[cfg_attr(not(target_os = "macos"), ignore = "xcode-select is only available on macOS")] +fn lookup_developer_dir() { + let _developer_dir = xcode_select_developer_dir().unwrap(); +} + +#[test] +#[cfg_attr(not(target_os = "macos"), ignore = "xcrun is only available on macOS")] +fn lookup_sdk() { + let (sdk_path, stderr) = xcrun_show_sdk_path("MacOSX", false).unwrap(); + // Check that the found SDK is valid. + assert!(sdk_path.join("SDKSettings.plist").exists()); + assert_eq!(stderr, ""); + + // Test that the SDK root is a subdir of the developer directory. + if let Some(developer_dir) = xcode_select_developer_dir() { + // Only run this test if SDKROOT is not set (otherwise xcrun may look up via. that). + if std::env::var_os("SDKROOT").is_some() { + assert!(sdk_path.starts_with(&developer_dir)); + } + } +} + +#[test] +#[cfg_attr(not(target_os = "macos"), ignore = "xcrun is only available on macOS")] +fn lookup_sdk_verbose() { + let (_, stderr) = xcrun_show_sdk_path("MacOSX", true).unwrap(); + // Newer xcrun versions should emit something like this: + // + // xcrun: note: looking up SDK with 'xcodebuild -sdk macosx -version Path' + // xcrun: note: xcrun_db = '/var/.../xcrun_db' + // xcrun: note: lookup resolved to: '...' + // xcrun: note: database key is: ... + // + // Or if the value is already cached, something like this: + // + // xcrun: note: database key is: ... + // xcrun: note: lookup resolved in '/var/.../xcrun_db' : '...' + assert!( + stderr.contains("xcrun: note: lookup resolved"), + "stderr should contain lookup note: {stderr}", + ); +} + +#[test] +#[cfg_attr(not(target_os = "macos"), ignore = "xcrun is only available on macOS")] +fn try_lookup_invalid_sdk() { + // As a proxy for testing all the different ways that `xcrun` can fail, + // test the case where an SDK was not found. + let err = xcrun_show_sdk_path("invalid", false).unwrap_err(); + let XcrunError::Unsuccessful { stderr, .. } = err else { + panic!("unexpected error kind: {err:?}"); + }; + // Either one of (depending on if using Command Line Tools or full Xcode): + // xcrun: error: SDK "invalid" cannot be located + // xcodebuild: error: SDK "invalid" cannot be located. + assert!( + stderr.contains(r#"error: SDK "invalid" cannot be located"#), + "stderr should contain xcodebuild note: {stderr}", + ); + assert!( + stderr.contains("xcrun: error: unable to lookup item 'Path' in SDK 'invalid'"), + "stderr should contain xcrun note: {stderr}", + ); +} diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index eb6aebf23203b..e66d549aabc4c 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -3213,15 +3213,7 @@ fn add_apple_sdk(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavor) -> return None; } - let sdk_name = apple::sdk_name(&sess.target).to_lowercase(); - - let sdk_root = match sess.time("get_apple_sdk_root", || get_apple_sdk_root(&sdk_name)) { - Ok(s) => s, - Err(e) => { - sess.dcx().emit_err(e); - return None; - } - }; + let sdk_root = sess.time("get_apple_sdk_root", || get_apple_sdk_root(sess))?; match flavor { LinkerFlavor::Darwin(Cc::Yes, _) => { @@ -3231,28 +3223,32 @@ fn add_apple_sdk(cmd: &mut dyn Linker, sess: &Session, flavor: LinkerFlavor) -> // This is admittedly a bit strange, as on most targets // `-isysroot` only applies to include header files, but on Apple // targets this also applies to libraries and frameworks. - cmd.cc_args(&["-isysroot", &sdk_root]); + cmd.cc_arg("-isysroot"); + cmd.cc_arg(&sdk_root); } LinkerFlavor::Darwin(Cc::No, _) => { - cmd.link_args(&["-syslibroot", &sdk_root]); + cmd.link_arg("-syslibroot"); + cmd.link_arg(&sdk_root); } _ => unreachable!(), } - Some(sdk_root.into()) + Some(sdk_root) } -fn get_apple_sdk_root(sdk_name: &str) -> Result> { - // Following what clang does - // (https://github.com/llvm/llvm-project/blob/ - // 296a80102a9b72c3eda80558fb78a3ed8849b341/clang/lib/Driver/ToolChains/Darwin.cpp#L1661-L1678) - // to allow the SDK path to be set. (For clang, xcrun sets - // SDKROOT; for rustc, the user or build system can set it, or we - // can fall back to checking for xcrun on PATH.) +fn get_apple_sdk_root(sess: &Session) -> Option { if let Ok(sdkroot) = env::var("SDKROOT") { - let p = Path::new(&sdkroot); - match sdk_name { - // Ignore `SDKROOT` if it's clearly set for the wrong platform. + let p = PathBuf::from(&sdkroot); + + // Ignore invalid SDKs, similar to what clang does: + // https://github.com/llvm/llvm-project/blob/llvmorg-19.1.6/clang/lib/Driver/ToolChains/Darwin.cpp#L2212-L2229 + // + // NOTE: Things are complicated here by the fact that `rustc` can be run by Cargo to compile + // build scripts and proc-macros for the host, and thus we need to ignore SDKROOT if it's + // clearly set for the wrong platform. + // + // FIXME(madsmtm): Make this more robust (maybe read `SDKSettings.json` like Clang does?). + match &*apple::sdk_name(&sess.target).to_lowercase() { "appletvos" if sdkroot.contains("TVSimulator.platform") || sdkroot.contains("MacOSX.platform") => {} @@ -3279,26 +3275,11 @@ fn get_apple_sdk_root(sdk_name: &str) -> Result {} // Ignore `SDKROOT` if it's not a valid path. _ if !p.is_absolute() || p == Path::new("/") || !p.exists() => {} - _ => return Ok(sdkroot), + _ => return Some(p), } } - let res = - Command::new("xcrun").arg("--show-sdk-path").arg("-sdk").arg(sdk_name).output().and_then( - |output| { - if output.status.success() { - Ok(String::from_utf8(output.stdout).unwrap()) - } else { - let error = String::from_utf8(output.stderr); - let error = format!("process exit with error: {}", error.unwrap()); - Err(io::Error::new(io::ErrorKind::Other, &error[..])) - } - }, - ); - match res { - Ok(output) => Ok(output.trim().to_string()), - Err(error) => Err(errors::AppleSdkRootError::SdkPath { sdk_name, error }), - } + apple::get_sdk_root(sess) } /// When using the linker flavors opting in to `lld`, add the necessary paths and arguments to diff --git a/compiler/rustc_codegen_ssa/src/errors.rs b/compiler/rustc_codegen_ssa/src/errors.rs index 9c4c205dfe552..f52d29baf9dc0 100644 --- a/compiler/rustc_codegen_ssa/src/errors.rs +++ b/compiler/rustc_codegen_ssa/src/errors.rs @@ -746,12 +746,6 @@ pub(crate) enum AppleDeploymentTarget { TooLow { env_var: &'static str, version: String, os_min: String }, } -#[derive(Diagnostic)] -pub(crate) enum AppleSdkRootError<'a> { - #[diag(codegen_ssa_apple_sdk_error_sdk_path)] - SdkPath { sdk_name: &'a str, error: Error }, -} - #[derive(Diagnostic)] #[diag(codegen_ssa_read_file)] pub(crate) struct ReadFileError { @@ -1327,3 +1321,26 @@ pub(crate) struct MixedExportNameAndNoMangle { #[suggestion(style = "verbose", code = "", applicability = "machine-applicable")] pub removal_span: Span, } + +#[derive(Diagnostic, Debug)] +pub(crate) enum XcrunError { + #[diag(codegen_ssa_xcrun_failed_invoking)] + FailedInvoking { sdk_name: &'static str, command_formatted: String, error: std::io::Error }, + + #[diag(codegen_ssa_xcrun_unsuccessful)] + #[note] + Unsuccessful { + sdk_name: &'static str, + command_formatted: String, + stdout: String, + stderr: String, + }, +} + +#[derive(Diagnostic, Debug)] +#[diag(codegen_ssa_xcrun_sdk_path_warning)] +#[note] +pub(crate) struct XcrunSdkPathWarning { + pub sdk_name: &'static str, + pub stderr: String, +} diff --git a/compiler/rustc_codegen_ssa/src/lib.rs b/compiler/rustc_codegen_ssa/src/lib.rs index 93c34a2f5764f..d26d6edf3149d 100644 --- a/compiler/rustc_codegen_ssa/src/lib.rs +++ b/compiler/rustc_codegen_ssa/src/lib.rs @@ -13,6 +13,7 @@ #![feature(let_chains)] #![feature(negative_impls)] #![feature(rustdoc_internals)] +#![feature(string_from_utf8_lossy_owned)] #![feature(trait_alias)] #![feature(try_blocks)] // tidy-alphabetical-end From 1f1c630a58297d49e92b79cf072559924b4f1123 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 5 Mar 2025 23:33:54 +0100 Subject: [PATCH 05/18] Greatly simplify doctest parsing and information extraction --- src/librustdoc/doctest/make.rs | 459 ++++++++++----------------------- 1 file changed, 136 insertions(+), 323 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 9935074877bc8..cb14608b35aeb 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -5,11 +5,10 @@ use std::fmt::{self, Write as _}; use std::io; use std::sync::Arc; -use rustc_ast as ast; +use rustc_ast::{self as ast, HasAttrs}; +use rustc_errors::ColorConfig; use rustc_errors::emitter::stderr_destination; -use rustc_errors::{ColorConfig, FatalError}; use rustc_parse::new_parser_from_source_str; -use rustc_parse::parser::attr::InnerAttrPolicy; use rustc_session::parse::ParseSess; use rustc_span::FileName; use rustc_span::edition::Edition; @@ -21,6 +20,19 @@ use super::GlobalTestOptions; use crate::display::Joined as _; use crate::html::markdown::LangString; +#[derive(Default)] +struct ParseSourceInfo { + has_main_fn: bool, + found_extern_crate: bool, + supports_color: bool, + has_global_allocator: bool, + has_macro_def: bool, + everything_else: String, + crates: String, + crate_attrs: String, + maybe_crate_attrs: String, +} + /// This struct contains information about the doctest itself which is then used to generate /// doctest source code appropriately. pub(crate) struct DocTestBuilder { @@ -53,8 +65,23 @@ impl DocTestBuilder { !lang_str.compile_fail && !lang_str.test_harness && !lang_str.standalone_crate }); - let Some(SourceInfo { crate_attrs, maybe_crate_attrs, crates, everything_else }) = - partition_source(source, edition) + let result = rustc_driver::catch_fatal_errors(|| { + rustc_span::create_session_if_not_set_then(edition, |_| { + parse_source(source, &crate_name) + }) + }); + + let Ok(Ok(ParseSourceInfo { + has_main_fn, + found_extern_crate, + supports_color, + has_global_allocator, + has_macro_def, + everything_else, + crates, + crate_attrs, + maybe_crate_attrs, + })) = result else { return Self::invalid( String::new(), @@ -65,35 +92,13 @@ impl DocTestBuilder { ); }; - // Uses librustc_ast to parse the doctest and find if there's a main fn and the extern - // crate already is included. - let Ok(( - ParseSourceInfo { - has_main_fn, - found_extern_crate, - supports_color, - has_global_allocator, - has_macro_def, - .. - }, - failed_ast, - )) = check_for_main_and_extern_crate( - crate_name, - source, - &everything_else, - &crates, - edition, - can_merge_doctests, - ) - else { - // If the parser panicked due to a fatal error, pass the test code through unchanged. - // The error will be reported during compilation. - return Self::invalid(crate_attrs, maybe_crate_attrs, crates, everything_else, test_id); - }; + debug!("crate_attrs:\n{crate_attrs}{maybe_crate_attrs}"); + debug!("crates:\n{crates}"); + debug!("after:\n{everything_else}"); + // If the AST returned an error, we don't want this doctest to be merged with the // others. Same if it contains `#[feature]` or `#[no_std]`. let can_be_merged = can_merge_doctests - && !failed_ast && !has_global_allocator && crate_attrs.is_empty() // If this is a merged doctest and a defined macro uses `$crate`, then the path will @@ -146,6 +151,7 @@ impl DocTestBuilder { if self.failed_ast { // If the AST failed to compile, no need to go generate a complete doctest, the error // will be better this way. + debug!("failed AST:\n{test_code}"); return (test_code.to_string(), 0); } let mut line_offset = 0; @@ -255,13 +261,6 @@ impl DocTestBuilder { } } -#[derive(PartialEq, Eq, Debug)] -enum ParsingResult { - Failed, - AstError, - Ok, -} - fn cancel_error_count(psess: &ParseSess) { // Reset errors so that they won't be reported as compiler bugs when dropping the // dcx. Any errors in the tests will be reported when the test file is compiled, @@ -270,17 +269,20 @@ fn cancel_error_count(psess: &ParseSess) { psess.dcx().reset_err_count(); } -fn parse_source( - source: String, - info: &mut ParseSourceInfo, - crate_name: &Option<&str>, -) -> ParsingResult { +const DOCTEST_CODE_WRAPPER: &str = "fn f(){"; + +fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { use rustc_errors::DiagCtxt; use rustc_errors::emitter::{Emitter, HumanEmitter}; - use rustc_parse::parser::ForceCollect; + // use rustc_parse::parser::ForceCollect; use rustc_span::source_map::FilePathMapping; - let filename = FileName::anon_source_code(&source); + let mut info = + ParseSourceInfo { found_extern_crate: crate_name.is_none(), ..Default::default() }; + + let wrapped_source = format!("{DOCTEST_CODE_WRAPPER}{source}\n}}"); + + let filename = FileName::anon_source_code(&wrapped_source); // Any errors in parsing should also appear when the doctest is compiled for real, so just // send all the errors that librustc_ast emits directly into a `Sink` instead of stderr. @@ -299,15 +301,32 @@ fn parse_source( let dcx = DiagCtxt::new(Box::new(emitter)).disable_warnings(); let psess = ParseSess::with_dcx(dcx, sm); - let mut parser = match new_parser_from_source_str(&psess, filename, source) { + let mut parser = match new_parser_from_source_str(&psess, filename, wrapped_source) { Ok(p) => p, Err(errs) => { errs.into_iter().for_each(|err| err.cancel()); cancel_error_count(&psess); - return ParsingResult::Failed; + return Err(()); } }; - let mut parsing_result = ParsingResult::Ok; + + fn push_to_s( + s: &mut String, + source: &str, + span: rustc_span::Span, + prev_span_hi: &mut Option, + ) { + let extra_len = DOCTEST_CODE_WRAPPER.len(); + // We need to shift by 1 because we added `{` at the beginning of the source.we provided + // to the parser. + let lo = prev_span_hi.unwrap_or(span.lo().0 as usize - extra_len); + let mut hi = span.hi().0 as usize - extra_len; + if hi > source.len() { + hi = source.len(); + } + s.push_str(&source[lo..hi]); + *prev_span_hi = Some(hi); + } // Recurse through functions body. It is necessary because the doctest source code is // wrapped in a function to limit the number of AST errors. If we don't recurse into @@ -325,6 +344,8 @@ fn parse_source( } match item.kind { ast::ItemKind::Fn(ref fn_item) if !info.has_main_fn => { + // We only push if it's the top item because otherwise, we would duplicate + // its content since the top-level item was already added. if item.ident.name == sym::main && is_top_level { info.has_main_fn = true; } @@ -334,7 +355,6 @@ fn parse_source( ast::StmtKind::Item(ref item) => { check_item(item, info, crate_name, false) } - ast::StmtKind::MacCall(..) => info.found_macro = true, _ => {} } } @@ -350,295 +370,88 @@ fn parse_source( }; } } - ast::ItemKind::MacCall(..) => info.found_macro = true, - ast::ItemKind::MacroDef(..) => info.has_macro_def = true, - _ => {} - } - } - - loop { - match parser.parse_item(ForceCollect::No) { - Ok(Some(item)) => { - check_item(&item, info, crate_name, true); - - if info.has_main_fn && info.found_extern_crate { - break; - } - } - Ok(None) => break, - Err(e) => { - parsing_result = ParsingResult::AstError; - e.cancel(); - break; + ast::ItemKind::MacroDef(..) => { + info.has_macro_def = true; } + _ => {} } - - // The supplied item is only used for diagnostics, - // which are swallowed here anyway. - parser.maybe_consume_incorrect_semicolon(None); - } - - cancel_error_count(&psess); - parsing_result -} - -#[derive(Default)] -struct ParseSourceInfo { - has_main_fn: bool, - found_extern_crate: bool, - found_macro: bool, - supports_color: bool, - has_global_allocator: bool, - has_macro_def: bool, -} - -fn check_for_main_and_extern_crate( - crate_name: Option<&str>, - original_source_code: &str, - everything_else: &str, - crates: &str, - edition: Edition, - can_merge_doctests: bool, -) -> Result<(ParseSourceInfo, bool), FatalError> { - let result = rustc_driver::catch_fatal_errors(|| { - rustc_span::create_session_if_not_set_then(edition, |_| { - let mut info = - ParseSourceInfo { found_extern_crate: crate_name.is_none(), ..Default::default() }; - - let mut parsing_result = - parse_source(format!("{crates}{everything_else}"), &mut info, &crate_name); - // No need to double-check this if the "merged doctests" feature isn't enabled (so - // before the 2024 edition). - if can_merge_doctests && parsing_result != ParsingResult::Ok { - // If we found an AST error, we want to ensure it's because of an expression being - // used outside of a function. - // - // To do so, we wrap in a function in order to make sure that the doctest AST is - // correct. For example, if your doctest is `foo::bar()`, if we don't wrap it in a - // block, it would emit an AST error, which would be problematic for us since we - // want to filter out such errors which aren't "real" errors. - // - // The end goal is to be able to merge as many doctests as possible as one for much - // faster doctests run time. - parsing_result = parse_source( - format!("{crates}\nfn __doctest_wrap(){{{everything_else}\n}}"), - &mut info, - &crate_name, - ); - } - - (info, parsing_result) - }) - }); - let (mut info, parsing_result) = match result { - Err(..) | Ok((_, ParsingResult::Failed)) => return Err(FatalError), - Ok((info, parsing_result)) => (info, parsing_result), - }; - - // If a doctest's `fn main` is being masked by a wrapper macro, the parsing loop above won't - // see it. In that case, run the old text-based scan to see if they at least have a main - // function written inside a macro invocation. See - // https://github.com/rust-lang/rust/issues/56898 - if info.found_macro - && !info.has_main_fn - && original_source_code - .lines() - .map(|line| { - let comment = line.find("//"); - if let Some(comment_begins) = comment { &line[0..comment_begins] } else { line } - }) - .any(|code| code.contains("fn main")) - { - info.has_main_fn = true; } - Ok((info, parsing_result != ParsingResult::Ok)) -} - -enum AttrKind { - CrateAttr, - Attr, -} - -/// Returns `Some` if the attribute is complete and `Some(true)` if it is an attribute that can be -/// placed at the crate root. -fn check_if_attr_is_complete(source: &str, edition: Edition) -> Option { - if source.is_empty() { - // Empty content so nothing to check in here... - return None; - } + let mut prev_span_hi = None; let not_crate_attrs = [sym::forbid, sym::allow, sym::warn, sym::deny]; + let parsed = parser.parse_item(rustc_parse::parser::ForceCollect::No); - rustc_driver::catch_fatal_errors(|| { - rustc_span::create_session_if_not_set_then(edition, |_| { - use rustc_errors::DiagCtxt; - use rustc_errors::emitter::HumanEmitter; - use rustc_span::source_map::FilePathMapping; - - let filename = FileName::anon_source_code(source); - // Any errors in parsing should also appear when the doctest is compiled for real, so just - // send all the errors that librustc_ast emits directly into a `Sink` instead of stderr. - let sm = Arc::new(SourceMap::new(FilePathMapping::empty())); - let fallback_bundle = rustc_errors::fallback_fluent_bundle( - rustc_driver::DEFAULT_LOCALE_RESOURCES.to_vec(), - false, - ); + debug!("+++++> {parsed:#?}"); - let emitter = HumanEmitter::new(Box::new(io::sink()), fallback_bundle); - - let dcx = DiagCtxt::new(Box::new(emitter)).disable_warnings(); - let psess = ParseSess::with_dcx(dcx, sm); - let mut parser = match new_parser_from_source_str(&psess, filename, source.to_owned()) { - Ok(p) => p, - Err(errs) => { - errs.into_iter().for_each(|err| err.cancel()); - // If there is an unclosed delimiter, an error will be returned by the - // tokentrees. - return None; - } - }; - // If a parsing error happened, it's very likely that the attribute is incomplete. - let ret = match parser.parse_attribute(InnerAttrPolicy::Permitted) { - Ok(attr) => { - let attr_name = attr.name_or_empty(); - - if not_crate_attrs.contains(&attr_name) { - // There is one exception to these attributes: - // `#![allow(internal_features)]`. If this attribute is used, we need to - // consider it only as a crate-level attribute. - if attr_name == sym::allow - && let Some(list) = attr.meta_item_list() - && list.iter().any(|sub_attr| { - sub_attr.name_or_empty().as_str() == "internal_features" - }) - { - Some(AttrKind::CrateAttr) - } else { - Some(AttrKind::Attr) - } + let result = match parsed { + Ok(Some(ref item)) + if let ast::ItemKind::Fn(ref fn_item) = item.kind + && let Some(ref body) = fn_item.body => + { + for attr in &item.attrs { + let attr_name = attr.name_or_empty(); + + if attr.style == ast::AttrStyle::Outer || not_crate_attrs.contains(&attr_name) { + // There is one exception to these attributes: + // `#![allow(internal_features)]`. If this attribute is used, we need to + // consider it only as a crate-level attribute. + if attr_name == sym::allow + && let Some(list) = attr.meta_item_list() + && list.iter().any(|sub_attr| { + sub_attr.name_or_empty().as_str() == "internal_features" + }) + { + push_to_s(&mut info.crate_attrs, source, attr.span, &mut prev_span_hi); } else { - Some(AttrKind::CrateAttr) + push_to_s( + &mut info.maybe_crate_attrs, + source, + attr.span, + &mut prev_span_hi, + ); } + } else { + push_to_s(&mut info.crate_attrs, source, attr.span, &mut prev_span_hi); } - Err(e) => { - e.cancel(); - None + } + for stmt in &body.stmts { + match stmt.kind { + ast::StmtKind::Item(ref item) => check_item(&item, &mut info, crate_name, true), + ast::StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => { + cancel_error_count(&psess); + return Err(()); + } + _ => {} } - }; - ret - }) - }) - .unwrap_or(None) -} -fn handle_attr(mod_attr_pending: &mut String, source_info: &mut SourceInfo, edition: Edition) { - if let Some(attr_kind) = check_if_attr_is_complete(mod_attr_pending, edition) { - let push_to = match attr_kind { - AttrKind::CrateAttr => &mut source_info.crate_attrs, - AttrKind::Attr => &mut source_info.maybe_crate_attrs, - }; - push_to.push_str(mod_attr_pending); - push_to.push('\n'); - // If it's complete, then we can clear the pending content. - mod_attr_pending.clear(); - } else { - mod_attr_pending.push('\n'); - } -} - -#[derive(Default)] -struct SourceInfo { - crate_attrs: String, - maybe_crate_attrs: String, - crates: String, - everything_else: String, -} - -fn partition_source(s: &str, edition: Edition) -> Option { - #[derive(Copy, Clone, PartialEq)] - enum PartitionState { - Attrs, - Crates, - Other, - } - let mut source_info = SourceInfo::default(); - let mut state = PartitionState::Attrs; - let mut mod_attr_pending = String::new(); - - for line in s.lines() { - let trimline = line.trim(); - - // FIXME(misdreavus): if a doc comment is placed on an extern crate statement, it will be - // shunted into "everything else" - match state { - PartitionState::Attrs => { - state = if trimline.starts_with("#![") { - mod_attr_pending = line.to_owned(); - handle_attr(&mut mod_attr_pending, &mut source_info, edition); - continue; - } else if trimline.chars().all(|c| c.is_whitespace()) - || (trimline.starts_with("//") && !trimline.starts_with("///")) - { - PartitionState::Attrs - } else if trimline.starts_with("extern crate") - || trimline.starts_with("#[macro_use] extern crate") - { - PartitionState::Crates - } else { - // First we check if the previous attribute was "complete"... - if !mod_attr_pending.is_empty() { - // If not, then we append the new line into the pending attribute to check - // if this time it's complete... - mod_attr_pending.push_str(line); - if !trimline.is_empty() { - handle_attr(&mut mod_attr_pending, &mut source_info, edition); - } - continue; - } else { - PartitionState::Other - } - }; - } - PartitionState::Crates => { - state = if trimline.starts_with("extern crate") - || trimline.starts_with("#[macro_use] extern crate") - || trimline.chars().all(|c| c.is_whitespace()) - || (trimline.starts_with("//") && !trimline.starts_with("///")) + // Weirdly enough, the `Stmt` span doesn't include its attributes, so we need to + // tweak the span to include the attributes as well. + let mut span = stmt.span; + if let Some(attr) = stmt.kind.attrs().first() { + span = span.with_lo(attr.span.lo()); + } + if info.everything_else.is_empty() + && (!info.maybe_crate_attrs.is_empty() || !info.crate_attrs.is_empty()) { - PartitionState::Crates - } else { - PartitionState::Other - }; + // We add potential backlines into attributes if there are some. + push_to_s( + &mut info.maybe_crate_attrs, + source, + span.shrink_to_lo(), + &mut prev_span_hi, + ); + } + push_to_s(&mut info.everything_else, source, span, &mut prev_span_hi); } - PartitionState::Other => {} + Ok(info) } - - match state { - PartitionState::Attrs => { - source_info.crate_attrs.push_str(line); - source_info.crate_attrs.push('\n'); - } - PartitionState::Crates => { - source_info.crates.push_str(line); - source_info.crates.push('\n'); - } - PartitionState::Other => { - source_info.everything_else.push_str(line); - source_info.everything_else.push('\n'); - } + Err(e) => { + e.cancel(); + Err(()) } - } - - if !mod_attr_pending.is_empty() { - debug!("invalid doctest code: {s:?}"); - return None; - } - - source_info.everything_else = source_info.everything_else.trim().to_string(); - - debug!("crate_attrs:\n{}{}", source_info.crate_attrs, source_info.maybe_crate_attrs); - debug!("crates:\n{}", source_info.crates); - debug!("after:\n{}", source_info.everything_else); + _ => Err(()), + }; - Some(source_info) + cancel_error_count(&psess); + result } From 01fb7c3ac6223ca5657987e73401f7eb64609e01 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 6 Mar 2025 16:44:59 +0100 Subject: [PATCH 06/18] Update rustdoc-ui tests --- tests/rustdoc-ui/extract-doctests.stdout | 2 +- tests/rustdoc-ui/remap-path-prefix-invalid-doctest.stdout | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/rustdoc-ui/extract-doctests.stdout b/tests/rustdoc-ui/extract-doctests.stdout index fa8604cae948a..b11531b844ee4 100644 --- a/tests/rustdoc-ui/extract-doctests.stdout +++ b/tests/rustdoc-ui/extract-doctests.stdout @@ -1 +1 @@ -{"format_version":1,"doctests":[{"file":"$DIR/extract-doctests.rs","line":8,"doctest_attributes":{"original":"ignore (checking attributes)","should_panic":false,"no_run":false,"ignore":"All","rust":true,"test_harness":false,"compile_fail":false,"standalone_crate":false,"error_codes":[],"edition":null,"added_css_classes":[],"unknown":[]},"original_code":"let x = 12;\nlet y = 14;","doctest_code":"#![allow(unused)]\nfn main() {\nlet x = 12;\nlet y = 14;\n}","name":"$DIR/extract-doctests.rs - (line 8)"},{"file":"$DIR/extract-doctests.rs","line":13,"doctest_attributes":{"original":"edition2018,compile_fail","should_panic":false,"no_run":true,"ignore":"None","rust":true,"test_harness":false,"compile_fail":true,"standalone_crate":false,"error_codes":[],"edition":"2018","added_css_classes":[],"unknown":[]},"original_code":"let","doctest_code":"#![allow(unused)]\nfn main() {\nlet\n}","name":"$DIR/extract-doctests.rs - (line 13)"}]} \ No newline at end of file +{"format_version":1,"doctests":[{"file":"$DIR/extract-doctests.rs","line":8,"doctest_attributes":{"original":"ignore (checking attributes)","should_panic":false,"no_run":false,"ignore":"All","rust":true,"test_harness":false,"compile_fail":false,"standalone_crate":false,"error_codes":[],"edition":null,"added_css_classes":[],"unknown":[]},"original_code":"let x = 12;\nlet y = 14;","doctest_code":"#![allow(unused)]\nfn main() {\nlet x = 12;\nlet y = 14;\n}","name":"$DIR/extract-doctests.rs - (line 8)"},{"file":"$DIR/extract-doctests.rs","line":13,"doctest_attributes":{"original":"edition2018,compile_fail","should_panic":false,"no_run":true,"ignore":"None","rust":true,"test_harness":false,"compile_fail":true,"standalone_crate":false,"error_codes":[],"edition":"2018","added_css_classes":[],"unknown":[]},"original_code":"let","doctest_code":null,"name":"$DIR/extract-doctests.rs - (line 13)"}]} \ No newline at end of file diff --git a/tests/rustdoc-ui/remap-path-prefix-invalid-doctest.stdout b/tests/rustdoc-ui/remap-path-prefix-invalid-doctest.stdout index a05b51699894b..c0d2515998f23 100644 --- a/tests/rustdoc-ui/remap-path-prefix-invalid-doctest.stdout +++ b/tests/rustdoc-ui/remap-path-prefix-invalid-doctest.stdout @@ -5,11 +5,11 @@ test remapped_path/remap-path-prefix-invalid-doctest.rs - SomeStruct (line 10) . failures: ---- remapped_path/remap-path-prefix-invalid-doctest.rs - SomeStruct (line 10) stdout ---- -error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `is` +error: expected one of `!` or `::`, found `is` --> remapped_path/remap-path-prefix-invalid-doctest.rs:11:6 | LL | this is not real code - | ^^ expected one of 8 possible tokens + | ^^ expected one of `!` or `::` error: aborting due to 1 previous error From 6f7e8d441a81ed89e14ad5ce53dcbe52ab0af64c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 7 Mar 2025 13:47:21 +0100 Subject: [PATCH 07/18] Correctly handle `fn main` in macro --- src/librustdoc/doctest/make.rs | 43 +++++++++++++++++++++++---------- src/librustdoc/doctest/tests.rs | 10 ++++---- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index cb14608b35aeb..810f53636ce41 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -5,15 +5,17 @@ use std::fmt::{self, Write as _}; use std::io; use std::sync::Arc; -use rustc_ast::{self as ast, HasAttrs}; +use rustc_ast::token::{Delimiter, TokenKind}; +use rustc_ast::tokenstream::TokenTree; +use rustc_ast::{self as ast, HasAttrs, StmtKind}; use rustc_errors::ColorConfig; use rustc_errors::emitter::stderr_destination; use rustc_parse::new_parser_from_source_str; use rustc_session::parse::ParseSess; -use rustc_span::FileName; use rustc_span::edition::Edition; use rustc_span::source_map::SourceMap; use rustc_span::symbol::sym; +use rustc_span::{FileName, kw}; use tracing::debug; use super::GlobalTestOptions; @@ -319,7 +321,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result source.len() { hi = source.len(); @@ -351,11 +353,8 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { - check_item(item, info, crate_name, false) - } - _ => {} + if let StmtKind::Item(ref item) = stmt.kind { + check_item(item, info, crate_name, false) } } } @@ -381,8 +380,6 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result {parsed:#?}"); - let result = match parsed { Ok(Some(ref item)) if let ast::ItemKind::Fn(ref fn_item) = item.kind @@ -416,11 +413,31 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result check_item(&item, &mut info, crate_name, true), - ast::StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => { + StmtKind::Item(ref item) => check_item(&item, &mut info, crate_name, true), + StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => { cancel_error_count(&psess); return Err(()); } + StmtKind::MacCall(ref mac_call) if !info.has_main_fn => { + let mut iter = mac_call.mac.args.tokens.iter(); + + while let Some(token) = iter.next() { + if let TokenTree::Token(token, _) = token + && let TokenKind::Ident(name, _) = token.kind + && name == kw::Fn + && let Some(TokenTree::Token(fn_token, _)) = iter.peek() + && let TokenKind::Ident(fn_name, _) = fn_token.kind + && fn_name == sym::main + && let Some(TokenTree::Delimited(_, _, Delimiter::Parenthesis, _)) = { + iter.next(); + iter.peek() + } + { + info.has_main_fn = true; + break; + } + } + } _ => {} } @@ -433,7 +450,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result Date: Fri, 7 Mar 2025 15:20:14 +0100 Subject: [PATCH 08/18] Correctly handle line comments in attributes and generate extern crates outside of wrapping function --- src/librustdoc/doctest/make.rs | 50 +++++++++++++------ src/librustdoc/doctest/tests.rs | 30 +++++++++-- tests/run-make/rustdoc-error-lines/rmake.rs | 6 +-- .../rustdoc-ui/doctest/display-output.stdout | 6 +-- tests/rustdoc-ui/doctest/extern-crate.rs | 23 +++++++++ tests/rustdoc-ui/doctest/extern-crate.stdout | 6 +++ tests/rustdoc/playground.rs | 2 +- 7 files changed, 98 insertions(+), 25 deletions(-) create mode 100644 tests/rustdoc-ui/doctest/extern-crate.rs create mode 100644 tests/rustdoc-ui/doctest/extern-crate.stdout diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 810f53636ce41..0ed2d37f74c18 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -176,9 +176,24 @@ impl DocTestBuilder { // Now push any outer attributes from the example, assuming they // are intended to be crate attributes. - prog.push_str(&self.crate_attrs); - prog.push_str(&self.maybe_crate_attrs); - prog.push_str(&self.crates); + if !self.crate_attrs.is_empty() { + prog.push_str(&self.crate_attrs); + if !self.crate_attrs.ends_with('\n') { + prog.push('\n'); + } + } + if !self.maybe_crate_attrs.is_empty() { + prog.push_str(&self.maybe_crate_attrs); + if !self.maybe_crate_attrs.ends_with('\n') { + prog.push('\n'); + } + } + if !self.crates.is_empty() { + prog.push_str(&self.crates); + if !self.crates.ends_with('\n') { + prog.push('\n'); + } + } // Don't inject `extern crate std` because it's already injected by the // compiler. @@ -276,7 +291,6 @@ const DOCTEST_CODE_WRAPPER: &str = "fn f(){"; fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { use rustc_errors::DiagCtxt; use rustc_errors::emitter::{Emitter, HumanEmitter}; - // use rustc_parse::parser::ForceCollect; use rustc_span::source_map::FilePathMapping; let mut info = @@ -338,7 +352,8 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result, is_top_level: bool, - ) { + ) -> bool { + let mut is_crate = false; if !info.has_global_allocator && item.attrs.iter().any(|attr| attr.name_or_empty() == sym::global_allocator) { @@ -354,12 +369,13 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { + is_crate = true; if !info.found_extern_crate && let Some(crate_name) = crate_name { @@ -374,6 +390,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result {} } + is_crate } let mut prev_span_hi = None; @@ -412,8 +429,11 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result check_item(&item, &mut info, crate_name, true), + StmtKind::Item(ref item) => { + is_crate = check_item(&item, &mut info, crate_name, true); + } StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => { cancel_error_count(&psess); return Err(()); @@ -450,15 +470,15 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result $DIR/display-output.rs:11:5 + --> $DIR/display-output.rs:12:5 | LL | let x = 12; | ^ help: if this is intentional, prefix it with an underscore: `_x` @@ -19,13 +19,13 @@ LL | #![warn(unused)] = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]` warning: unused variable: `x` - --> $DIR/display-output.rs:13:8 + --> $DIR/display-output.rs:14:8 | LL | fn foo(x: &dyn std::fmt::Display) {} | ^ help: if this is intentional, prefix it with an underscore: `_x` warning: function `foo` is never used - --> $DIR/display-output.rs:13:4 + --> $DIR/display-output.rs:14:4 | LL | fn foo(x: &dyn std::fmt::Display) {} | ^^^ diff --git a/tests/rustdoc-ui/doctest/extern-crate.rs b/tests/rustdoc-ui/doctest/extern-crate.rs new file mode 100644 index 0000000000000..0415d33bb723b --- /dev/null +++ b/tests/rustdoc-ui/doctest/extern-crate.rs @@ -0,0 +1,23 @@ +//@ check-pass +//@ compile-flags:--test --test-args=--test-threads=1 +//@ normalize-stdout: "tests/rustdoc-ui/doctest" -> "$$DIR" +//@ normalize-stdout: "finished in \d+\.\d+s" -> "finished in $$TIME" + +// This test ensures that crate imports are placed outside of the `main` function +// so they work all the time (even in 2015 edition). + +/// ```rust +/// #![feature(test)] +/// +/// extern crate test; +/// use test::Bencher; +/// +/// #[bench] +/// fn bench_xor_1000_ints(b: &mut Bencher) { +/// b.iter(|| { +/// (0..1000).fold(0, |old, new| old ^ new); +/// }); +/// } +/// ``` +/// +pub fn foo() {} diff --git a/tests/rustdoc-ui/doctest/extern-crate.stdout b/tests/rustdoc-ui/doctest/extern-crate.stdout new file mode 100644 index 0000000000000..b103343afdd57 --- /dev/null +++ b/tests/rustdoc-ui/doctest/extern-crate.stdout @@ -0,0 +1,6 @@ + +running 1 test +test $DIR/extern-crate.rs - foo (line 9) ... ok + +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME + diff --git a/tests/rustdoc/playground.rs b/tests/rustdoc/playground.rs index db2d1669df604..65dad2a5195d6 100644 --- a/tests/rustdoc/playground.rs +++ b/tests/rustdoc/playground.rs @@ -24,4 +24,4 @@ //@ matches foo/index.html '//a[@class="test-arrow"][@href="https://www.example.com/?code=%23!%5Ballow(unused)%5D%0Afn+main()+%7B%0A++++println!(%22Hello,+world!%22);%0A%7D&edition=2015"]' "" //@ matches foo/index.html '//a[@class="test-arrow"][@href="https://www.example.com/?code=%23!%5Ballow(unused)%5D%0Afn+main()+%7B%0A++++println!(%22Hello,+world!%22);%0A%7D&edition=2015"]' "" -//@ matches foo/index.html '//a[@class="test-arrow"][@href="https://www.example.com/?code=%23!%5Ballow(unused)%5D%0A%23!%5Bfeature(something)%5D%0A%0Afn+main()+%7B%0A++++println!(%22Hello,+world!%22);%0A%7D&version=nightly&edition=2015"]' "" +//@ matches foo/index.html '//a[@class="test-arrow"][@href="https://www.example.com/?code=%23!%5Ballow(unused)%5D%0A%23!%5Bfeature(something)%5D%0A%0A%0Afn+main()+%7B%0A++++println!(%22Hello,+world!%22);%0A%7D&version=nightly&edition=2015"]' "" From be13105d737545a4a5f0aa0709342b4db8f2f1e3 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 27 Mar 2025 12:55:30 +0100 Subject: [PATCH 09/18] Improve comment and test for generated doctest with code comments --- src/librustdoc/doctest/make.rs | 5 +++-- src/librustdoc/doctest/tests.rs | 24 +++++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 0ed2d37f74c18..512343693bc28 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -85,6 +85,8 @@ impl DocTestBuilder { maybe_crate_attrs, })) = result else { + // If the AST returned an error, we don't want this doctest to be merged with the + // others. return Self::invalid( String::new(), String::new(), @@ -98,8 +100,7 @@ impl DocTestBuilder { debug!("crates:\n{crates}"); debug!("after:\n{everything_else}"); - // If the AST returned an error, we don't want this doctest to be merged with the - // others. Same if it contains `#[feature]` or `#[no_std]`. + // If it contains `#[feature]` or `#[no_std]`, we don't want it to be merged either. let can_be_merged = can_merge_doctests && !has_global_allocator && crate_attrs.is_empty() diff --git a/src/librustdoc/doctest/tests.rs b/src/librustdoc/doctest/tests.rs index 59cc33558ed50..4833ea0405154 100644 --- a/src/librustdoc/doctest/tests.rs +++ b/src/librustdoc/doctest/tests.rs @@ -405,7 +405,9 @@ fn check_split_args() { #[test] fn comment_in_attrs() { - // if input already has a fn main, it should insert a space before it + // If there is an inline code comment after attributes, we need to ensure that + // a backline will be added to prevent generating code "inside" it (and thus generating) + // invalid code. let opts = default_global_opts(""); let input = "\ #![feature(rustdoc_internals)] @@ -424,4 +426,24 @@ fn main() { .to_string(); let (output, len) = make_test(input, None, false, &opts, None); assert_eq!((output, len), (expected, 2)); + + // And same, if there is a `main` function provided by the user, we ensure that it's + // correctly separated. + let input = "\ +#![feature(rustdoc_internals)] +#![allow(internal_features)] +#![doc(rust_logo)] +//! This crate has the Rust(tm) branding on it. +fn main() {}"; + let expected = "\ +#![allow(unused)] +#![feature(rustdoc_internals)] +#![allow(internal_features)] +#![doc(rust_logo)] +//! This crate has the Rust(tm) branding on it. + +fn main() {}" + .to_string(); + let (output, len) = make_test(input, None, false, &opts, None); + assert_eq!((output, len), (expected, 1)); } From f333ad8656af7a43559805a9a321da8cf618d0c1 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 27 Mar 2025 13:00:49 +0100 Subject: [PATCH 10/18] Improve code comment --- src/librustdoc/doctest/make.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 512343693bc28..46226a8bc7174 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -334,8 +334,8 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result, ) { let extra_len = DOCTEST_CODE_WRAPPER.len(); - // We need to shift by 1 because we added `{` at the beginning of the source.we provided - // to the parser. + // We need to shift by the length of `DOCTEST_CODE_WRAPPER` because we + // added it at the beginning of the source we provided to the parser. let lo = prev_span_hi.unwrap_or(0); let mut hi = span.hi().0 as usize - extra_len; if hi > source.len() { From 8327bb6ad3c6a6975ed7a0a408614b8d90c8d0ad Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 27 Mar 2025 13:01:25 +0100 Subject: [PATCH 11/18] Add `expect` to the list of non-crate attributes for doctest generation --- src/librustdoc/doctest/make.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 46226a8bc7174..abb626025a8c8 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -395,7 +395,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result Date: Thu, 27 Mar 2025 14:32:29 +0100 Subject: [PATCH 12/18] Improve code --- src/librustdoc/doctest/make.rs | 63 ++++++++++++++++------------------ 1 file changed, 29 insertions(+), 34 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index abb626025a8c8..93be352000607 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -25,7 +25,7 @@ use crate::html::markdown::LangString; #[derive(Default)] struct ParseSourceInfo { has_main_fn: bool, - found_extern_crate: bool, + already_has_extern_crate: bool, supports_color: bool, has_global_allocator: bool, has_macro_def: bool, @@ -48,7 +48,7 @@ pub(crate) struct DocTestBuilder { pub(crate) crates: String, pub(crate) everything_else: String, pub(crate) test_id: Option, - pub(crate) failed_ast: bool, + pub(crate) invalid_ast: bool, pub(crate) can_be_merged: bool, } @@ -75,7 +75,7 @@ impl DocTestBuilder { let Ok(Ok(ParseSourceInfo { has_main_fn, - found_extern_crate, + already_has_extern_crate, supports_color, has_global_allocator, has_macro_def, @@ -114,9 +114,9 @@ impl DocTestBuilder { maybe_crate_attrs, crates, everything_else, - already_has_extern_crate: found_extern_crate, + already_has_extern_crate, test_id, - failed_ast: false, + invalid_ast: false, can_be_merged, } } @@ -137,7 +137,7 @@ impl DocTestBuilder { everything_else, already_has_extern_crate: false, test_id, - failed_ast: true, + invalid_ast: true, can_be_merged: false, } } @@ -151,10 +151,10 @@ impl DocTestBuilder { opts: &GlobalTestOptions, crate_name: Option<&str>, ) -> (String, usize) { - if self.failed_ast { + if self.invalid_ast { // If the AST failed to compile, no need to go generate a complete doctest, the error // will be better this way. - debug!("failed AST:\n{test_code}"); + debug!("invalid AST:\n{test_code}"); return (test_code.to_string(), 0); } let mut line_offset = 0; @@ -279,7 +279,7 @@ impl DocTestBuilder { } } -fn cancel_error_count(psess: &ParseSess) { +fn reset_error_count(psess: &ParseSess) { // Reset errors so that they won't be reported as compiler bugs when dropping the // dcx. Any errors in the tests will be reported when the test file is compiled, // Note that we still need to cancel the errors above otherwise `Diag` will panic on @@ -295,7 +295,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result) -> Result p, Err(errs) => { errs.into_iter().for_each(|err| err.cancel()); - cancel_error_count(&psess); + reset_error_count(&psess); return Err(()); } }; - fn push_to_s( - s: &mut String, - source: &str, - span: rustc_span::Span, - prev_span_hi: &mut Option, - ) { + fn push_to_s(s: &mut String, source: &str, span: rustc_span::Span, prev_span_hi: &mut usize) { let extra_len = DOCTEST_CODE_WRAPPER.len(); // We need to shift by the length of `DOCTEST_CODE_WRAPPER` because we // added it at the beginning of the source we provided to the parser. - let lo = prev_span_hi.unwrap_or(0); let mut hi = span.hi().0 as usize - extra_len; if hi > source.len() { hi = source.len(); } - s.push_str(&source[lo..hi]); - *prev_span_hi = Some(hi); + s.push_str(&source[*prev_span_hi..hi]); + *prev_span_hi = hi; } // Recurse through functions body. It is necessary because the doctest source code is @@ -354,7 +348,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result, is_top_level: bool, ) -> bool { - let mut is_crate = false; + let mut is_extern_crate = false; if !info.has_global_allocator && item.attrs.iter().any(|attr| attr.name_or_empty() == sym::global_allocator) { @@ -370,17 +364,17 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { - is_crate = true; - if !info.found_extern_crate + is_extern_crate = true; + if !info.already_has_extern_crate && let Some(crate_name) = crate_name { - info.found_extern_crate = match original { + info.already_has_extern_crate = match original { Some(name) => name.as_str() == *crate_name, None => item.ident.as_str() == *crate_name, }; @@ -391,10 +385,10 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result {} } - is_crate + is_extern_crate } - let mut prev_span_hi = None; + let mut prev_span_hi = 0; let not_crate_attrs = [sym::forbid, sym::allow, sym::warn, sym::deny, sym::expect]; let parsed = parser.parse_item(rustc_parse::parser::ForceCollect::No); @@ -430,13 +424,13 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { - is_crate = check_item(&item, &mut info, crate_name, true); + is_extern_crate = check_item(&item, &mut info, crate_name, true); } StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => { - cancel_error_count(&psess); + reset_error_count(&psess); return Err(()); } StmtKind::MacCall(ref mac_call) if !info.has_main_fn => { @@ -471,11 +465,12 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result) -> Result Err(()), }; - cancel_error_count(&psess); + reset_error_count(&psess); result } From 3371d498b1d6853598b9bedaf1e71d4c3f1d538b Mon Sep 17 00:00:00 2001 From: joboet Date: Thu, 27 Mar 2025 15:41:46 +0100 Subject: [PATCH 13/18] std: get rid of pre-Vista fallback code We haven't had any Windows XP targets for a long while now... --- library/std/src/sys/pal/windows/pipe.rs | 28 ++++++------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/library/std/src/sys/pal/windows/pipe.rs b/library/std/src/sys/pal/windows/pipe.rs index 8521cf4162f5c..c785246492268 100644 --- a/library/std/src/sys/pal/windows/pipe.rs +++ b/library/std/src/sys/pal/windows/pipe.rs @@ -74,7 +74,6 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res let ours; let mut name; let mut tries = 0; - let mut reject_remote_clients_flag = c::PIPE_REJECT_REMOTE_CLIENTS; loop { tries += 1; name = format!( @@ -96,7 +95,7 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res c::PIPE_TYPE_BYTE | c::PIPE_READMODE_BYTE | c::PIPE_WAIT - | reject_remote_clients_flag, + | c::PIPE_REJECT_REMOTE_CLIENTS, 1, PIPE_BUFFER_CAPACITY, PIPE_BUFFER_CAPACITY, @@ -112,30 +111,15 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res // // Don't try again too much though as this could also perhaps be a // legit error. - // If `ERROR_INVALID_PARAMETER` is returned, this probably means we're - // running on pre-Vista version where `PIPE_REJECT_REMOTE_CLIENTS` is - // not supported, so we continue retrying without it. This implies - // reduced security on Windows versions older than Vista by allowing - // connections to this pipe from remote machines. - // Proper fix would increase the number of FFI imports and introduce - // significant amount of Windows XP specific code with no clean - // testing strategy - // For more info, see https://github.com/rust-lang/rust/pull/37677. if handle == c::INVALID_HANDLE_VALUE { let error = api::get_last_error(); - if tries < 10 { - if error == WinError::ACCESS_DENIED { - continue; - } else if reject_remote_clients_flag != 0 - && error == WinError::INVALID_PARAMETER - { - reject_remote_clients_flag = 0; - tries -= 1; - continue; - } + if tries < 10 && error == WinError::ACCESS_DENIED { + continue; + } else { + return Err(io::Error::from_raw_os_error(error.code as i32)); } - return Err(io::Error::from_raw_os_error(error.code as i32)); } + ours = Handle::from_raw_handle(handle); break; } From 1494da4ffbfab8d0b7a62dd37d9f4a77f1038840 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 27 Mar 2025 16:23:41 +0100 Subject: [PATCH 14/18] Add new regression test for doctest --- src/librustdoc/doctest/tests.rs | 131 ++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/src/librustdoc/doctest/tests.rs b/src/librustdoc/doctest/tests.rs index 4833ea0405154..949ed837db3af 100644 --- a/src/librustdoc/doctest/tests.rs +++ b/src/librustdoc/doctest/tests.rs @@ -447,3 +447,134 @@ fn main() {}" let (output, len) = make_test(input, None, false, &opts, None); assert_eq!((output, len), (expected, 1)); } + +#[test] +fn comments() { + let opts = default_global_opts(""); + let input = r##" +//! A doc comment that applies to the implicit anonymous module of this crate + +pub mod outer_module { + + //! - Inner line doc + //!! - Still an inner line doc (but with a bang at the beginning) + + /*! - Inner block doc */ + /*!! - Still an inner block doc (but with a bang at the beginning) */ + + // - Only a comment + /// - Outer line doc (exactly 3 slashes) + //// - Only a comment + + /* - Only a comment */ + /** - Outer block doc (exactly) 2 asterisks */ + /*** - Only a comment */ + + pub mod inner_module {} + + pub mod nested_comments { + /* In Rust /* we can /* nest comments */ */ */ + + // All three types of block comments can contain or be nested inside + // any other type: + + /* /* */ /** */ /*! */ */ + /*! /* */ /** */ /*! */ */ + /** /* */ /** */ /*! */ */ + pub mod dummy_item {} + } + + pub mod degenerate_cases { + // empty inner line doc + //! + + // empty inner block doc + /*!*/ + + // empty line comment + // + + // empty outer line doc + /// + + // empty block comment + /**/ + + pub mod dummy_item {} + + // empty 2-asterisk block isn't a doc block, it is a block comment + /***/ + + } + + /* The next one isn't allowed because outer doc comments + require an item that will receive the doc */ + + /// Where is my item? +} +"##; + let expected = " +//! A doc comment that applies to the implicit anonymous module of this crate + +pub mod outer_module { + + //! - Inner line doc + //!! - Still an inner line doc (but with a bang at the beginning) + + /*! - Inner block doc */ + /*!! - Still an inner block doc (but with a bang at the beginning) */ + + // - Only a comment + /// - Outer line doc (exactly 3 slashes) + //// - Only a comment + + /* - Only a comment */ + /** - Outer block doc (exactly) 2 asterisks */ + /*** - Only a comment */ + + pub mod inner_module {} + + pub mod nested_comments { + /* In Rust /* we can /* nest comments */ */ */ + + // All three types of block comments can contain or be nested inside + // any other type: + + /* /* */ /** */ /*! */ */ + /*! /* */ /** */ /*! */ */ + /** /* */ /** */ /*! */ */ + pub mod dummy_item {} + } + + pub mod degenerate_cases { + // empty inner line doc + //! + + // empty inner block doc + /*!*/ + + // empty line comment + // + + // empty outer line doc + /// + + // empty block comment + /**/ + + pub mod dummy_item {} + + // empty 2-asterisk block isn't a doc block, it is a block comment + /***/ + + } + + /* The next one isn't allowed because outer doc comments + require an item that will receive the doc */ + + /// Where is my item? +} +".to_string(); + let (output, len) = make_test(input, None, false, &opts, None); + assert_eq!((output, len), (expected, 0)); +} \ No newline at end of file From 49f1e9cd8d31b65416408f4975feb5eded4d6158 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 27 Mar 2025 14:43:11 +0100 Subject: [PATCH 15/18] Remove recursion in `check_item` --- src/librustdoc/doctest/make.rs | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 93be352000607..55b57292b33a2 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -342,12 +342,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result, - is_top_level: bool, - ) -> bool { + fn check_item(item: &ast::Item, info: &mut ParseSourceInfo, crate_name: &Option<&str>) -> bool { let mut is_extern_crate = false; if !info.has_global_allocator && item.attrs.iter().any(|attr| attr.name_or_empty() == sym::global_allocator) @@ -355,19 +350,12 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { + ast::ItemKind::Fn(_) if !info.has_main_fn => { // We only push if it's the top item because otherwise, we would duplicate // its content since the top-level item was already added. - if item.ident.name == sym::main && is_top_level { + if item.ident.name == sym::main { info.has_main_fn = true; } - if let Some(ref body) = fn_item.body { - for stmt in &body.stmts { - if let StmtKind::Item(ref item) = stmt.kind { - is_extern_crate |= check_item(item, info, crate_name, false); - } - } - } } ast::ItemKind::ExternCrate(original) => { is_extern_crate = true; @@ -427,7 +415,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result { - is_extern_crate = check_item(&item, &mut info, crate_name, true); + is_extern_crate = check_item(&item, &mut info, crate_name); } StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => { reset_error_count(&psess); From 5d274408d4b8b845f490912dcdadcc717f18fcca Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 27 Mar 2025 17:43:29 +0100 Subject: [PATCH 16/18] Only take outer attributes into account when generating content between first non-crate items and the crate items --- src/librustdoc/doctest/make.rs | 8 +- src/librustdoc/doctest/tests.rs | 131 ++++---------------------------- 2 files changed, 19 insertions(+), 120 deletions(-) diff --git a/src/librustdoc/doctest/make.rs b/src/librustdoc/doctest/make.rs index 55b57292b33a2..56b1e76ae8cfd 100644 --- a/src/librustdoc/doctest/make.rs +++ b/src/librustdoc/doctest/make.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use rustc_ast::token::{Delimiter, TokenKind}; use rustc_ast::tokenstream::TokenTree; -use rustc_ast::{self as ast, HasAttrs, StmtKind}; +use rustc_ast::{self as ast, AttrStyle, HasAttrs, StmtKind}; use rustc_errors::ColorConfig; use rustc_errors::emitter::stderr_destination; use rustc_parse::new_parser_from_source_str; @@ -388,7 +388,7 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result) -> Result Date: Thu, 27 Mar 2025 18:29:06 +0000 Subject: [PATCH 17/18] Use `abs_diff` where applicable --- compiler/rustc_errors/src/snippet.rs | 6 +----- compiler/rustc_span/src/edit_distance.rs | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_errors/src/snippet.rs b/compiler/rustc_errors/src/snippet.rs index 8485d7087cfc5..f09c2ed5534a5 100644 --- a/compiler/rustc_errors/src/snippet.rs +++ b/compiler/rustc_errors/src/snippet.rs @@ -159,11 +159,7 @@ impl Annotation { /// Length of this annotation as displayed in the stderr output pub(crate) fn len(&self) -> usize { // Account for usize underflows - if self.end_col.display > self.start_col.display { - self.end_col.display - self.start_col.display - } else { - self.start_col.display - self.end_col.display - } + self.end_col.display.abs_diff(self.start_col.display) } pub(crate) fn has_label(&self) -> bool { diff --git a/compiler/rustc_span/src/edit_distance.rs b/compiler/rustc_span/src/edit_distance.rs index 8a2baaa42e2a3..4f3202b694c1c 100644 --- a/compiler/rustc_span/src/edit_distance.rs +++ b/compiler/rustc_span/src/edit_distance.rs @@ -118,7 +118,7 @@ pub fn edit_distance_with_substrings(a: &str, b: &str, limit: usize) -> Option Date: Thu, 27 Mar 2025 15:40:03 -0400 Subject: [PATCH 18/18] saethlin goes on vacation --- triagebot.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/triagebot.toml b/triagebot.toml index ebbcfa4516b99..7e1e1bc577161 100644 --- a/triagebot.toml +++ b/triagebot.toml @@ -1098,6 +1098,7 @@ contributing_url = "https://rustc-dev-guide.rust-lang.org/getting-started.html" users_on_vacation = [ "jyn514", "ChrisDenton", + "saethlin", ] [[assign.warn_non_default_branch.exceptions]]