Merge pull request #17608 from squeek502/resinator-fixes

resinator: Fix `INCLUDE` var handling and sync with upstream

Author: andrewrk

lib/std/Build/Step/Compile.zig

@@ -244,6 +244,8 @@ pub const RcSourceFile = struct {
     file: LazyPath,
     /// Any option that rc.exe accepts will work here, with the exception of:
     /// - `/fo`: The output filename is set by the build system
+    /// - `/p`: Only running the preprocessor is not supported in this context
+    /// - `/:no-preprocess` (non-standard option): Not supported in this context
     /// - Any MUI-related option
     /// https://learn.microsoft.com/en-us/windows/win32/menurc/using-rc-the-rc-command-line-
     ///

src/Compilation.zig

@@ -4766,11 +4766,21 @@ fn updateWin32Resource(comp: *Compilation, win32_resource: *Win32Resource, win32
         };
         defer options.deinit();
 
+        // We never want to read the INCLUDE environment variable, so
+        // unconditionally set `ignore_include_env_var` to true
+        options.ignore_include_env_var = true;
+
+        if (options.preprocess != .yes) {
+            return comp.failWin32Resource(win32_resource, "the '{s}' option is not supported in this context", .{switch (options.preprocess) {
+                .no => "/:no-preprocess",
+                .only => "/p",
+                .yes => unreachable,
+            }});
+        }
+
         var argv = std.ArrayList([]const u8).init(comp.gpa);
         defer argv.deinit();
 
-        // TODO: support options.preprocess == .no and .only
-        //       alternatively, error if those options are used
         try argv.appendSlice(&[_][]const u8{ self_exe_path, "clang" });
 
         try resinator.preprocess.appendClangArgs(arena, &argv, options, .{

src/introspect.zig

@@ -157,8 +157,6 @@ pub const EnvVar = enum {
     NO_COLOR,
     XDG_CACHE_HOME,
     HOME,
-    /// https://github.com/ziglang/zig/issues/17585
-    INCLUDE,
 
     pub fn isSet(comptime ev: EnvVar) bool {
         return std.process.hasEnvVarConstant(@tagName(ev));

src/resinator/compile.zig

@@ -28,7 +28,6 @@ const windows1252 = @import("windows1252.zig");
 const lang = @import("lang.zig");
 const code_pages = @import("code_pages.zig");
 const errors = @import("errors.zig");
-const introspect = @import("../introspect.zig");
 
 pub const CompileOptions = struct {
     cwd: std.fs.Dir,
@@ -89,10 +88,23 @@ pub fn compile(allocator: Allocator, source: []const u8, writer: anytype, option
         }
     }
     // Re-open the passed in cwd since we want to be able to close it (std.fs.cwd() shouldn't be closed)
-    // `catch unreachable` since `options.cwd` is expected to be a valid dir handle, so opening
-    // a new handle to it should be fine as well.
-    // TODO: Maybe catch and return an error instead
-    const cwd_dir = options.cwd.openDir(".", .{}) catch @panic("unable to open dir");
+    const cwd_dir = options.cwd.openDir(".", .{}) catch |err| {
+        try options.diagnostics.append(.{
+            .err = .failed_to_open_cwd,
+            .token = .{
+                .id = .invalid,
+                .start = 0,
+                .end = 0,
+                .line_number = 1,
+            },
+            .print_source_line = false,
+            .extra = .{ .file_open_error = .{
+                .err = ErrorDetails.FileOpenError.enumFromError(err),
+                .filename_string_index = undefined,
+            } },
+        });
+        return error.CompileError;
+    };
     try search_dirs.append(.{ .dir = cwd_dir, .path = null });
     for (options.extra_include_paths) |extra_include_path| {
         var dir = openSearchPathDir(options.cwd, extra_include_path) catch {
@@ -111,11 +123,16 @@ pub fn compile(allocator: Allocator, source: []const u8, writer: anytype, option
         try search_dirs.append(.{ .dir = dir, .path = try allocator.dupe(u8, system_include_path) });
     }
     if (!options.ignore_include_env_var) {
-        const INCLUDE = (introspect.EnvVar.INCLUDE.get(allocator) catch @panic("OOM")) orelse "";
+        const INCLUDE = std.process.getEnvVarOwned(allocator, "INCLUDE") catch "";
         defer allocator.free(INCLUDE);
 
-        // TODO: Should this be platform-specific? How does windres/llvm-rc handle this (if at all)?
-        var it = std.mem.tokenize(u8, INCLUDE, ";");
+        // The only precedence here is llvm-rc which also uses the platform-specific
+        // delimiter. There's no precedence set by `rc.exe` since it's Windows-only.
+        const delimiter = switch (builtin.os.tag) {
+            .windows => ';',
+            else => ':',
+        };
+        var it = std.mem.tokenizeScalar(u8, INCLUDE, delimiter);
         while (it.next()) |search_path| {
             var dir = openSearchPathDir(options.cwd, search_path) catch continue;
             errdefer dir.close();

src/resinator/errors.zig

@@ -395,6 +395,10 @@ pub const ErrorDetails = struct {
         // General (used in various places)
         /// `number` is populated and contains the value that the ordinal would have in the Win32 RC compiler implementation
         win32_non_ascii_ordinal,
+
+        // Initialization
+        /// `file_open_error` is populated, but `filename_string_index` is not
+        failed_to_open_cwd,
     };
 
     pub fn render(self: ErrorDetails, writer: anytype, source: []const u8, strings: []const []const u8) !void {
@@ -766,6 +770,9 @@ pub const ErrorDetails = struct {
                 .note => return writer.print("the Win32 RC compiler would accept this as an ordinal but its value would be {}", .{self.extra.number}),
                 .hint => return,
             },
+            .failed_to_open_cwd => {
+                try writer.print("failed to open CWD for compilation: {s}", .{@tagName(self.extra.file_open_error.err)});
+            },
         }
     }
 
@@ -804,7 +811,8 @@ pub const ErrorDetails = struct {
                 .point_offset = self.token.start - source_line_start,
                 .after_len = after: {
                     const end = @min(source_line_end, if (self.token_span_end) |span_end| span_end.end else self.token.end);
-                    if (end == self.token.start) break :after 0;
+                    // end may be less than start when pointing to EOF
+                    if (end <= self.token.start) break :after 0;
                     break :after end - self.token.start - 1;
                 },
             },
@@ -816,13 +824,18 @@ pub fn renderErrorMessage(allocator: std.mem.Allocator, writer: anytype, tty_con
     if (err_details.type == .hint) return;
 
     const source_line_start = err_details.token.getLineStart(source);
-    const column = err_details.token.calculateColumn(source, 1, source_line_start);
-
-    // var counting_writer_container = std.io.countingWriter(writer);
-    // const counting_writer = counting_writer_container.writer();
-
-    const corresponding_span: ?SourceMappings.SourceSpan = if (source_mappings) |mappings| mappings.get(err_details.token.line_number) else null;
-    const corresponding_file: ?[]const u8 = if (source_mappings) |mappings| mappings.files.get(corresponding_span.?.filename_offset) else null;
+    // Treat tab stops as 1 column wide for error display purposes,
+    // and add one to get a 1-based column
+    const column = err_details.token.calculateColumn(source, 1, source_line_start) + 1;
+
+    const corresponding_span: ?SourceMappings.SourceSpan = if (source_mappings != null and source_mappings.?.has(err_details.token.line_number))
+        source_mappings.?.get(err_details.token.line_number)
+    else
+        null;
+    const corresponding_file: ?[]const u8 = if (source_mappings != null and corresponding_span != null)
+        source_mappings.?.files.get(corresponding_span.?.filename_offset)
+    else
+        null;
 
     const err_line = if (corresponding_span) |span| span.start_line else err_details.token.line_number;
 
@@ -897,7 +910,7 @@ pub fn renderErrorMessage(allocator: std.mem.Allocator, writer: anytype, tty_con
     try writer.writeByte('\n');
     try tty_config.setColor(writer, .reset);
 
-    if (source_mappings) |_| {
+    if (corresponding_span != null and corresponding_file != null) {
         var corresponding_lines = try CorrespondingLines.init(allocator, cwd, err_details, source_line_for_display_buf.items, corresponding_span.?, corresponding_file.?);
         defer corresponding_lines.deinit(allocator);