Skip to content

Commit

Permalink
feat: Warn about unreferenced imports
Browse files Browse the repository at this point in the history
closes #272
  • Loading branch information
giann committed Mar 12, 2024
1 parent d09b954 commit a8f0c4f
Show file tree
Hide file tree
Showing 9 changed files with 110 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ var value = from {
```
- `recursive_call_limit` build option limit recursive calls (default to 200)
- Compiler will warn about code after a `return` statement
- Compiler will warn about unreferenced imports (https://github.com/buzz-language/buzz/issues/272)
- `namespace` (https://github.com/buzz-language/buzz/issues/271): if a script exports at least one symbol, it has to define a namespace for the script with `namespace mynamespace`
- By default, imported symbols from another script will be under `libprefix.XXXX`
- When importing something, you can still redefine its namespace prefix with `import "..." as mynewnamespace` or remove it altogether with `import "..." _`
Expand Down
108 changes: 98 additions & 10 deletions src/Parser.zig
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,14 @@ pub const Global = struct {
name_token: Ast.TokenIndex,
location: Ast.TokenIndex,
type_def: *obj.ObjTypeDef,
export_alias: ?[]const u8 = null,
imported_from: ?[]const u8 = null,

initialized: bool = false,
exported: bool = false,
export_alias: ?[]const u8 = null,
hidden: bool = false,
constant: bool,
referenced: bool = false,
// When resolving a placeholder, the start of the resolution is the global
// If `constant` is true, we can search for any `.Assignment` link and fail then.

pub fn isReferenced(self: Global) bool {
const function_type = if (self.type_def.def_type == .Function)
Expand Down Expand Up @@ -266,6 +266,11 @@ pub const ScriptImport = struct {
absolute_path: *obj.ObjString,
};

const LocalScriptImport = struct {
referenced: bool = false,
location: Ast.TokenIndex,
};

pub var user_library_paths: ?[][]const u8 = null;

pub const DeclarationTerminator = enum {
Expand Down Expand Up @@ -473,8 +478,10 @@ script_name: []const u8 = undefined,
imported: bool = false,
// True when parsing a declaration inside an export statement
exporting: bool = false,
// Cached imported functions
// Cached imported functions (shared across instances of Parser)
imports: *std.StringHashMap(ScriptImport),
// Keep track of things imported by the current script
script_imports: std.StringHashMap(LocalScriptImport),
test_count: u64 = 0,
// FIXME: use SinglyLinkedList instead of heap allocated ptrs
current: ?*Frame = null,
Expand All @@ -497,6 +504,7 @@ pub fn init(
var self = Self{
.gc = gc,
.imports = imports,
.script_imports = std.StringHashMap(LocalScriptImport).init(gc.allocator),
.imported = imported,
.globals = std.ArrayList(Global).init(gc.allocator),
.flavor = flavor,
Expand All @@ -515,6 +523,7 @@ pub fn init(

pub fn deinit(self: *Self) void {
self.globals.deinit();
self.script_imports.deinit();
if (self.opt_jumps) |jumps| {
jumps.deinit();
}
Expand Down Expand Up @@ -893,6 +902,21 @@ pub fn parse(self: *Self, source: []const u8, file_name: []const u8) !?Ast {
}
}

// Check there's no unreferenced imports
var it = self.script_imports.iterator();
while (it.next()) |kv| {
if (!kv.value_ptr.*.referenced) {
const location = self.ast.tokens.get(kv.value_ptr.*.location);

self.reporter.warnFmt(
.unused_import,
location,
"Unused import",
.{},
);
}
}

self.ast.nodes.items(.components)[function_node].Function.entry = entry;

self.ast.root = if (self.reporter.had_error) null else self.endFrame();
Expand Down Expand Up @@ -2956,8 +2980,22 @@ fn parseUserType(self: *Self, instance: bool) Error!Ast.Node.Index {

// Search for a global with that name
if (try self.resolveGlobal(null, self.ast.tokens.items(.lexeme)[user_type_name])) |slot| {
var_type = self.globals.items[slot].type_def;
const global = self.globals.items[slot];

var_type = global.type_def;
global_slot = @intCast(slot);

if (global.imported_from != null and self.script_imports.get(global.imported_from.?) != null) {
const imported_from = global.imported_from.?;

try self.script_imports.put(
imported_from,
.{
.location = self.script_imports.get(imported_from).?.location,
.referenced = true,
},
);
}
}

// If none found, create a placeholder
Expand Down Expand Up @@ -4729,10 +4767,24 @@ fn namedVariable(self: *Self, name: Ast.TokenIndex, can_assign: bool) Error!Ast.
slot_type = .UpValue;
slot_constant = self.current.?.enclosing.?.locals[self.current.?.upvalues[uslot].index].constant;
} else if (try self.resolveGlobal(null, self.ast.tokens.items(.lexeme)[name])) |uslot| {
var_def = self.globals.items[uslot].type_def;
const global = self.globals.items[uslot];

var_def = global.type_def;
slot = uslot;
slot_type = .Global;
slot_constant = self.globals.items[uslot].constant;
slot_constant = global.constant;

if (global.imported_from != null and self.script_imports.get(global.imported_from.?) != null) {
const imported_from = global.imported_from.?;

try self.script_imports.put(
imported_from,
.{
.location = self.script_imports.get(imported_from).?.location,
.referenced = true,
},
);
}
} else {
slot = try self.declarePlaceholder(name, null);
var_def = self.globals.items[slot].type_def;
Expand Down Expand Up @@ -5794,10 +5846,22 @@ fn exportStatement(self: *Self) Error!Ast.Node.Index {
// Search for a global with that name
if (try self.resolveGlobal(null, self.ast.tokens.items(.lexeme)[identifier])) |slot| {
const global = &self.globals.items[slot];
global.referenced = true;
var alias: ?Ast.TokenIndex = null;

global.referenced = true;
global.exported = true;
if (global.imported_from != null and self.script_imports.get(global.imported_from.?) != null) {
const imported_from = global.imported_from.?;

try self.script_imports.put(
imported_from,
.{
.location = self.script_imports.get(imported_from).?.location,
.referenced = true,
},
);
}

var alias: ?Ast.TokenIndex = null;
if (try self.match(.As)) {
try self.consume(.Identifier, "Expected identifier after `as`.");

Expand Down Expand Up @@ -7123,6 +7187,7 @@ fn readScript(self: *Self, file_name: []const u8) !?[2][]const u8 {

fn importScript(
self: *Self,
path_token: Ast.TokenIndex,
file_name: []const u8,
prefix: ?[]const u8,
imported_symbols: *std.StringHashMap(Ast.Node.Index),
Expand Down Expand Up @@ -7178,6 +7243,12 @@ fn importScript(
}

try self.imports.put(file_name, import.?);
try self.script_imports.put(
file_name,
.{
.location = path_token,
},
);
}

// Caught up this parser with the import parser status
Expand Down Expand Up @@ -7219,6 +7290,8 @@ fn importScript(
prefix orelse global.prefix;
}

global.imported_from = file_name;

// TODO: we're forced to import all and hide some because globals are indexed and not looked up by name at runtime
// Only way to avoid this is to go back to named globals at runtime. Then again, is it worth it?
try self.globals.append(global);
Expand Down Expand Up @@ -7414,6 +7487,7 @@ fn importStatement(self: *Self) Error!Ast.Node.Index {

const import = if (!self.reporter.had_error)
try self.importScript(
path_token,
file_name,
if (prefix) |pr|
self.ast.tokens.items(.lexeme)[pr]
Expand Down Expand Up @@ -7618,7 +7692,21 @@ fn userVarDeclaration(self: *Self, _: bool, constant: bool) Error!Ast.Node.Index
// Search for a global with that name
if (var_type == null) {
if (try self.resolveGlobal(null, self.ast.tokens.items(.lexeme)[user_type_name])) |slot| {
var_type = self.globals.items[slot].type_def;
const global = self.globals.items[slot];

var_type = global.type_def;

if (global.imported_from != null and self.script_imports.get(global.imported_from.?) != null) {
const imported_from = global.imported_from.?;

try self.script_imports.put(
imported_from,
.{
.location = self.script_imports.get(imported_from).?.location,
.referenced = true,
},
);
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/Reporter.zig
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ pub const Error = enum(u8) {
empty_import = 91,
import_already_exists = 92,
code_after_return = 93,
unused_import = 94,
};

// Inspired by https://github.com/zesterer/ariadne
Expand Down
3 changes: 1 addition & 2 deletions tests/009-gc.buzz
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import "std";
import "gc" as gc;
import "debug";
import "gc";

bool collectorCalled = false;

Expand Down
5 changes: 2 additions & 3 deletions tests/032-debug.buzz
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import "std";
import "debug" as debug;
import "serialize";
import "debug";
| import "serialize";

| TODO: put back one debug.ast is reactivated
| test "Get AST" {
Expand Down
1 change: 0 additions & 1 deletion tests/058-ffi.buzz
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import "std";
import "buffer" as _;
import "debug";
import "ffi";

| TODO: one zdef for one lib and multiple declarations
Expand Down
1 change: 0 additions & 1 deletion tests/066-object-generics.buzz
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import "std";
import "debug";

object Payload::<K, V> {
{K: V} data,
Expand Down
1 change: 0 additions & 1 deletion tests/069-named-expr.buzz
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import "std";
import "debug";

object Person {
str name,
Expand Down
7 changes: 7 additions & 0 deletions tests/compile_errors/028-unused-import.buzz
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
| Unused import
import "math";
import "std";

test "Doing nothing with the math import" {
std.print("hello");
}

0 comments on commit a8f0c4f

Please sign in to comment.