Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add diagnostics for CMake ConfigHeader errors #20538

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+130 −41
Open

Add diagnostics for CMake ConfigHeader errors #20538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 129 additions & 39 deletions lib/std/Build/Step/ConfigHeader.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ const ConfigHeader = @This();
const Step = std.Build.Step;
const Allocator = std.mem.Allocator;

const ConfigError = error{
InvalidCharacter,
MissingName,
UndefinedVariable,
};

pub const Style = union(enum) {
/// The configure format supported by autotools. It uses `#undef foo` to
/// mark lines that can be substituted with different values.
Expand All @@ -23,6 +29,37 @@ pub const Style = union(enum) {
}
};

/// Diagnostic data wrapper, complementing returned `ConfigError`s
const Diagnostics = struct {
allocator: std.mem.Allocator,

/// 1-based line number
line_number: u64 = 1,
/// 1-based column number, indicates start column of the error
column_number: usize = 1,
/// optional metadata accompanying the `ConfigError`
metadata: ?[]u8 = null,

pub fn deinit(self: *@This()) void {
if (self.metadata) |m| {
self.allocator.free(m);
}
self.metadata = undefined;
}

/// Set or reset metadata, should be called whenever `ConfigError` is being returned
pub fn setMetadata(self: *@This(), value: ?[]const u8) void {
if (self.metadata) |m| {
self.allocator.free(m);
self.metadata = null;
}

if (value) |v| {
self.metadata = self.allocator.dupe(u8, v) catch null;
}
}
};

pub const Value = union(enum) {
undef,
defined,
Expand All @@ -40,6 +77,7 @@ style: Style,
max_bytes: usize,
include_path: []const u8,
include_guard_override: ?[]const u8,
fallback_value: ?Value,

pub const base_id: Step.Id = .config_header;

Expand All @@ -49,6 +87,7 @@ pub const Options = struct {
include_path: ?[]const u8 = null,
first_ret_addr: ?usize = null,
include_guard_override: ?[]const u8 = null,
fallback_value: ?Value = null,
};

pub fn create(owner: *std.Build, options: Options) *ConfigHeader {
Expand Down Expand Up @@ -94,6 +133,7 @@ pub fn create(owner: *std.Build, options: Options) *ConfigHeader {
.max_bytes = options.max_bytes,
.include_path = include_path,
.include_guard_override = options.include_guard_override,
.fallback_value = options.fallback_value,
.output_file = .{ .step = &config_header.step },
};

Expand Down Expand Up @@ -209,7 +249,7 @@ fn make(step: *Step, options: Step.MakeOptions) !void {
src_path, @errorName(err),
});
};
try render_cmake(step, contents, &output, config_header.values, src_path);
try render_cmake(step, contents, &output, config_header.values, config_header.fallback_value, src_path);
},
.blank => {
try output.appendSlice(c_generated_line);
Expand Down Expand Up @@ -309,35 +349,36 @@ fn render_cmake(
contents: []const u8,
output: *std.ArrayList(u8),
values: std.StringArrayHashMap(Value),
fallback_value: ?Value,
src_path: []const u8,
) !void {
const build = step.owner;
const allocator = build.allocator;

var values_copy = try values.clone();
defer values_copy.deinit();

var any_errors = false;
var line_index: u32 = 0;
var line_it = std.mem.splitScalar(u8, contents, '\n');
while (line_it.next()) |raw_line| : (line_index += 1) {

var arena = std.heap.ArenaAllocator.init(allocator);
defer arena.deinit();
const arena_allocator = arena.allocator();

var unused_values = try values.cloneWithAllocator(arena_allocator);
var diagnostics: Diagnostics = .{ .allocator = arena_allocator };

while (line_it.next()) |raw_line| : (diagnostics.line_number += 1) {
diagnostics.column_number = 1;
const last_line = line_it.index == line_it.buffer.len;

const line = expand_variables_cmake(allocator, raw_line, values) catch |err| switch (err) {
error.InvalidCharacter => {
try step.addError("{s}:{d}: error: invalid character in a variable name", .{
src_path, line_index + 1,
});
any_errors = true;
continue;
},
else => {
try step.addError("{s}:{d}: unable to substitute variable: error: {s}", .{
src_path, line_index + 1, @errorName(err),
});
any_errors = true;
continue;
},
const line = expand_variables_cmake(allocator, raw_line, values, &unused_values, fallback_value, &diagnostics) catch |err| {
try step.addError("{s}:{d}:{d}: unable to substitute variable: {s}: {s}", .{
src_path,
diagnostics.line_number,
diagnostics.column_number,
@errorName(err),
diagnostics.metadata orelse "",
});
any_errors = true;
continue;
};
defer allocator.free(line);

Expand All @@ -364,17 +405,18 @@ fn render_cmake(

const name = it.next() orelse {
try step.addError("{s}:{d}: error: missing define name", .{
src_path, line_index + 1,
src_path, diagnostics.line_number,
});
any_errors = true;
continue;
};
var value = values_copy.get(name) orelse blk: {
var value = values.get(name) orelse blk: {
if (booldefine) {
break :blk Value{ .int = 0 };
}
break :blk Value.undef;
};
_ = unused_values.swapRemove(name);

value = blk: {
switch (value) {
Expand Down Expand Up @@ -430,6 +472,13 @@ fn render_cmake(
try renderValueC(output, name, value);
}

if (unused_values.keys().len > 0) {
try step.addError("{s}: error: unused variables: {s}", .{
src_path, unused_values.keys(),
});
any_errors = true;
}

if (any_errors) {
return error.HeaderConfigFailed;
}
Expand Down Expand Up @@ -540,6 +589,9 @@ fn expand_variables_cmake(
allocator: Allocator,
contents: []const u8,
values: std.StringArrayHashMap(Value),
unused_values: *std.StringArrayHashMap(Value),
fallback_value: ?Value,
diagnostics: *Diagnostics,
) ![]const u8 {
var result = std.ArrayList(u8).init(allocator);
errdefer result.deinit();
Expand Down Expand Up @@ -570,7 +622,15 @@ fn expand_variables_cmake(
}

const key = contents[curr + 1 .. close_pos];
const value = values.get(key) orelse return error.MissingValue;

errdefer {
diagnostics.column_number = curr + 1;
diagnostics.setMetadata(key);
}

const value = values.get(key) orelse fallback_value orelse return ConfigError.UndefinedVariable;
_ = unused_values.swapRemove(key);

const missing = contents[source_offset..curr];
try result.appendSlice(missing);
switch (value) {
Expand Down Expand Up @@ -617,6 +677,9 @@ fn expand_variables_cmake(
break :blk;
}
const open_pos = var_stack.pop();
errdefer {
diagnostics.column_number = open_pos.source;
}
if (source_offset == open_pos.source) {
source_offset += open_var.len;
}
Expand All @@ -626,9 +689,14 @@ fn expand_variables_cmake(
const key_start = open_pos.target + open_var.len;
const key = result.items[key_start..];
if (key.len == 0) {
return error.MissingKey;
diagnostics.setMetadata(null);
return ConfigError.MissingName;
}
const value = values.get(key) orelse return error.MissingValue;
const value = values.get(key) orelse fallback_value orelse {
diagnostics.setMetadata(key);
return ConfigError.UndefinedVariable;
};
_ = unused_values.swapRemove(key);
result.shrinkRetainingCapacity(result.items.len - key.len - open_var.len);
switch (value) {
.undef, .defined => {},
Expand All @@ -655,7 +723,9 @@ fn expand_variables_cmake(
}

if (var_stack.items.len > 0 and std.mem.indexOfScalar(u8, valid_varname_chars, contents[curr]) == null) {
return error.InvalidCharacter;
diagnostics.column_number = curr + 1;
diagnostics.setMetadata(&[_]u8{contents[curr]});
return ConfigError.InvalidCharacter;
}
}

Expand All @@ -673,7 +743,23 @@ fn testReplaceVariables(
expected: []const u8,
values: std.StringArrayHashMap(Value),
) !void {
const actual = try expand_variables_cmake(allocator, contents, values);
try testReplaceVariablesWithFallback(allocator, contents, expected, values, null);
}

fn testReplaceVariablesWithFallback(
allocator: Allocator,
contents: []const u8,
expected: []const u8,
values: std.StringArrayHashMap(Value),
fallback_value: ?Value,
) !void {
var diagnostics: Diagnostics = .{ .allocator = allocator };
defer diagnostics.deinit();

var unused = try values.clone();
defer unused.deinit();

const actual = try expand_variables_cmake(allocator, contents, values, &unused, fallback_value, &diagnostics);
defer allocator.free(actual);

try std.testing.expectEqualStrings(expected, actual);
Expand All @@ -699,7 +785,7 @@ test "expand_variables_cmake simple cases" {
try testReplaceVariables(allocator, "no substitution", "no substitution", values);

// empty ${} wrapper leads to an error
try std.testing.expectError(error.MissingKey, testReplaceVariables(allocator, "${}", "", values));
try std.testing.expectError(ConfigError.MissingName, testReplaceVariables(allocator, "${}", "", values));

// empty @ sigils are preserved
try testReplaceVariables(allocator, "@", "@", values);
Expand Down Expand Up @@ -763,8 +849,12 @@ test "expand_variables_cmake simple cases" {
try testReplaceVariables(allocator, "undef}", "undef}", values);

// unknown key leads to an error
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "@bad@", "", values));
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "${bad}", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "@bad@", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "${bad}", "", values));

// fallback_value suppresses an error
try testReplaceVariablesWithFallback(allocator, "@bad@", "fallback", values, .{ .string = "fallback" });
try testReplaceVariablesWithFallback(allocator, "${bad}", "fallback", values, .{ .string = "fallback" });
}

test "expand_variables_cmake edge cases" {
Expand Down Expand Up @@ -809,23 +899,23 @@ test "expand_variables_cmake edge cases" {
try testReplaceVariables(allocator, "@dollar@{@string@}", "${text}", values);

// when expanded variables contain invalid characters, they prevent further expansion
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "${${string_var}}", "", values));
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "${@string_var@}", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "${${string_var}}", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "${@string_var@}", "", values));

// nested expanded variables are expanded from the inside out
try testReplaceVariables(allocator, "${string${underscore}proxy}", "string", values);
try testReplaceVariables(allocator, "${string@underscore@proxy}", "string", values);

// nested vars are only expanded when ${} is closed
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "@nest@underscore@proxy@", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "@nest@underscore@proxy@", "", values));
try testReplaceVariables(allocator, "${nest${underscore}proxy}", "nest_underscore_proxy", values);
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "@nest@@nest_underscore@underscore@proxy@@proxy@", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "@nest@@nest_underscore@underscore@proxy@@proxy@", "", values));
try testReplaceVariables(allocator, "${nest${${nest_underscore${underscore}proxy}}proxy}", "nest_underscore_proxy", values);

// invalid characters lead to an error
try std.testing.expectError(error.InvalidCharacter, testReplaceVariables(allocator, "${str*ing}", "", values));
try std.testing.expectError(error.InvalidCharacter, testReplaceVariables(allocator, "${str$ing}", "", values));
try std.testing.expectError(error.InvalidCharacter, testReplaceVariables(allocator, "${str@ing}", "", values));
try std.testing.expectError(ConfigError.InvalidCharacter, testReplaceVariables(allocator, "${str*ing}", "", values));
try std.testing.expectError(ConfigError.InvalidCharacter, testReplaceVariables(allocator, "${str$ing}", "", values));
try std.testing.expectError(ConfigError.InvalidCharacter, testReplaceVariables(allocator, "${str@ing}", "", values));
}

test "expand_variables_cmake escaped characters" {
Expand All @@ -845,5 +935,5 @@ test "expand_variables_cmake escaped characters" {
try testReplaceVariables(allocator, "$\\{string}", "$\\{string}", values);

// backslash is skipped when checking for invalid characters, yet it mangles the key
try std.testing.expectError(error.MissingValue, testReplaceVariables(allocator, "${string\\}", "", values));
try std.testing.expectError(ConfigError.UndefinedVariable, testReplaceVariables(allocator, "${string\\}", "", values));
}
3 changes: 1 addition & 2 deletions test/standalone/cmakedefine/build.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ pub fn build(b: *std.Build) void {
.include_path = "stack.h",
},
.{
.AT = "@",
.UNDERSCORE = "_",
.NEST_UNDERSCORE_PROXY = "UNDERSCORE",
.NEST_PROXY = "NEST_UNDERSCORE_PROXY",
Expand Down Expand Up @@ -104,7 +103,7 @@ fn compare_headers(step: *std.Build.Step, options: std.Build.Step.MakeOptions) !
const header_text_index = std.mem.indexOf(u8, zig_header, "\n") orelse @panic("Could not find comment in header filer");

if (!std.mem.eql(u8, zig_header[header_text_index + 1 ..], cmake_header)) {
@panic("processed cmakedefine header does not match expected output");
std.debug.panic("processed cmakedefine header {s} does not match expected output", .{zig_header_path});
}
}
}
Loading