From 4b0e56f848a44f7bd25b51866d9d34a0fbd25c85 Mon Sep 17 00:00:00 2001 From: ike709 Date: Fri, 20 Sep 2024 15:53:54 -0500 Subject: [PATCH 1/5] Resolve proc override paths in var values --- .../Procs/procpath_without_proc_element.dm | 11 +++++ DMCompiler/DM/Expressions/Constant.cs | 44 +++++++++++++------ 2 files changed, 41 insertions(+), 14 deletions(-) create mode 100644 Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm diff --git a/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm new file mode 100644 index 0000000000..16d1be651f --- /dev/null +++ b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm @@ -0,0 +1,11 @@ +// RETURN TRUE + +// This test confirms we don't OD0404 if a var is set to a procpath that does not contain the "/proc/" element + +/atom/movable/proc/foo() + return + +/datum/var/bar = /atom/movable/foo + +/proc/RunTest() + return TRUE diff --git a/DMCompiler/DM/Expressions/Constant.cs b/DMCompiler/DM/Expressions/Constant.cs index 6b4cb135d9..4706f09d51 100644 --- a/DMCompiler/DM/Expressions/Constant.cs +++ b/DMCompiler/DM/Expressions/Constant.cs @@ -336,17 +336,9 @@ public bool TryResolvePath([NotNullWhen(true)] out (PathType Type, int Id)? path if (procIndex != -1) { DreamPath withoutProcElement = path.RemoveElement(procIndex); DreamPath ownerPath = withoutProcElement.FromElements(0, -2); - DMObject owner = DMObjectTree.GetDMObject(ownerPath, createIfNonexistent: false); - string procName = path.LastElement; + string procName = path.LastElement!; - int? procId; - if (owner == DMObjectTree.Root && DMObjectTree.TryGetGlobalProc(procName, out var globalProc)) { - procId = globalProc.Id; - } else { - var procs = owner.GetProcs(procName); - - procId = procs?[^1]; - } + ResolveProc(ownerPath, procName, out var procId); if (procId == null) { DMCompiler.Emit(WarningCode.ItemDoesntExist, Location, @@ -364,11 +356,35 @@ public bool TryResolvePath([NotNullWhen(true)] out (PathType Type, int Id)? path if (DMObjectTree.TryGetTypeId(Value, out var typeId)) { pathInfo = (PathType.TypeReference, typeId); return true; - } else { - DMCompiler.Emit(WarningCode.ItemDoesntExist, Location, $"Type {Value} does not exist"); + } - pathInfo = null; - return false; + // If it's not a type, check again for a proc but without the /proc/ element (which is valid) + if (ResolveProc(path.FromElements(0, -2), path.LastElement!, out var proc)) { + pathInfo = (PathType.ProcReference, proc.Value); + return true; + } + + DMCompiler.Emit(WarningCode.ItemDoesntExist, Location, $"Type {Value} does not exist"); + + pathInfo = null; + return false; + + bool ResolveProc(DreamPath ownerPath, string procName, [NotNullWhen(true)] out int? procId) { + procId = null; + + DMObject? owner = DMObjectTree.GetDMObject(ownerPath, createIfNonexistent: false); + if (owner is null) return false; + + if (owner == DMObjectTree.Root && DMObjectTree.TryGetGlobalProc(procName, out var globalProc)) { + procId = globalProc.Id; + return true; + } + + var procs = owner.GetProcs(procName); + if (procs is null || procs.Count == 0) return false; + + procId = procs[^1]; + return true; } } } From 9d41ec32f91486abf13449dab4156a87a9335002 Mon Sep 17 00:00:00 2001 From: ike709 Date: Fri, 20 Sep 2024 16:22:18 -0500 Subject: [PATCH 2/5] Cope with lateral overrides --- .../Tests/Procs/ambiguous_procpath_error.dm | 12 ++++++++++++ .../Tests/Procs/ambiguous_procpath_noerror.dm | 11 +++++++++++ DMCompiler/Compiler/CompilerError.cs | 1 + DMCompiler/DM/Expressions/Constant.cs | 19 ++++++++++++++----- DMCompiler/DMStandard/DefaultPragmaConfig.dm | 1 + 5 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm create mode 100644 Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_noerror.dm diff --git a/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm b/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm new file mode 100644 index 0000000000..782ac330cf --- /dev/null +++ b/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm @@ -0,0 +1,12 @@ +// COMPILE ERROR +#pragma AmbiguousProcPath error + +/datum/proc/foo() + return + +/datum/foo() + return + +/proc/RunTest() + var/meep = /datum/proc/foo + return diff --git a/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_noerror.dm b/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_noerror.dm new file mode 100644 index 0000000000..046e1e6316 --- /dev/null +++ b/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_noerror.dm @@ -0,0 +1,11 @@ +#pragma AmbiguousProcPath error + +/datum/proc/foo() + return + +/datum/foo() + return + +/proc/RunTest() + var/meep = /datum/foo + return diff --git a/DMCompiler/Compiler/CompilerError.cs b/DMCompiler/Compiler/CompilerError.cs index ed7f43bd81..d15de5d723 100644 --- a/DMCompiler/Compiler/CompilerError.cs +++ b/DMCompiler/Compiler/CompilerError.cs @@ -57,6 +57,7 @@ public enum WarningCode { DanglingVarType = 2401, // For types inferred by a particular var definition and nowhere else, that ends up not existing (not forced-fatal because BYOND doesn't always error) MissingInterpolatedExpression = 2500, // A text macro is missing a required interpolated expression AmbiguousResourcePath = 2600, + AmbiguousProcPath = 2601, // A procpath is being referenced with a /proc/ element even though lateral overrides exist and will be ignored UnsupportedTypeCheck = 2700, InvalidReturnType = 2701, // Proc static typing InvalidVarType = 2702, // Var static typing diff --git a/DMCompiler/DM/Expressions/Constant.cs b/DMCompiler/DM/Expressions/Constant.cs index 4706f09d51..dcdfcdc456 100644 --- a/DMCompiler/DM/Expressions/Constant.cs +++ b/DMCompiler/DM/Expressions/Constant.cs @@ -338,7 +338,7 @@ public bool TryResolvePath([NotNullWhen(true)] out (PathType Type, int Id)? path DreamPath ownerPath = withoutProcElement.FromElements(0, -2); string procName = path.LastElement!; - ResolveProc(ownerPath, procName, out var procId); + ResolveProc(ownerPath, procName, false, out var procId); if (procId == null) { DMCompiler.Emit(WarningCode.ItemDoesntExist, Location, @@ -358,8 +358,8 @@ public bool TryResolvePath([NotNullWhen(true)] out (PathType Type, int Id)? path return true; } - // If it's not a type, check again for a proc but without the /proc/ element (which is valid) - if (ResolveProc(path.FromElements(0, -2), path.LastElement!, out var proc)) { + // If it's not a type, check again for a proc override without the /proc/ element + if (ResolveProc(path.FromElements(0, -2), path.LastElement!, true, out var proc)) { pathInfo = (PathType.ProcReference, proc.Value); return true; } @@ -369,7 +369,7 @@ public bool TryResolvePath([NotNullWhen(true)] out (PathType Type, int Id)? path pathInfo = null; return false; - bool ResolveProc(DreamPath ownerPath, string procName, [NotNullWhen(true)] out int? procId) { + bool ResolveProc(DreamPath ownerPath, string procName, bool isOverride, [NotNullWhen(true)] out int? procId) { procId = null; DMObject? owner = DMObjectTree.GetDMObject(ownerPath, createIfNonexistent: false); @@ -383,7 +383,16 @@ bool ResolveProc(DreamPath ownerPath, string procName, [NotNullWhen(true)] out i var procs = owner.GetProcs(procName); if (procs is null || procs.Count == 0) return false; - procId = procs[^1]; + if (isOverride) { + procId = procs[^1]; + } else { + procId = procs[0]; + if (procs.Count > 1) { + DMCompiler.Emit(WarningCode.AmbiguousProcPath, Location, + $"Type {ownerPath} has lateral overrides of proc {procName} but \"/proc/\" references the original definition only"); + } + } + return true; } } diff --git a/DMCompiler/DMStandard/DefaultPragmaConfig.dm b/DMCompiler/DMStandard/DefaultPragmaConfig.dm index b35b4632c4..4398de7653 100644 --- a/DMCompiler/DMStandard/DefaultPragmaConfig.dm +++ b/DMCompiler/DMStandard/DefaultPragmaConfig.dm @@ -29,6 +29,7 @@ #pragma DanglingVarType warning #pragma MissingInterpolatedExpression warning #pragma AmbiguousResourcePath warning +#pragma AmbiguousProcPath warning #pragma SuspiciousSwitchCase warning #pragma PointlessPositionalArgument warning // NOTE: The next few pragmas are for OpenDream's experimental type checker From b2e4adf3a77534bdbf53cd544876e3754a21344b Mon Sep 17 00:00:00 2001 From: ike709 Date: Tue, 1 Oct 2024 12:12:06 -0500 Subject: [PATCH 3/5] fix not erroring if there's no override --- .../Tests/Procs/procpath_without_proc_element.dm | 7 +++---- .../Tests/Procs/procpath_without_proc_element2.dm | 13 +++++++++++++ DMCompiler/DM/Expressions/Constant.cs | 5 +++++ 3 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element2.dm diff --git a/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm index 16d1be651f..9ab0465949 100644 --- a/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm +++ b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm @@ -1,11 +1,10 @@ -// RETURN TRUE - -// This test confirms we don't OD0404 if a var is set to a procpath that does not contain the "/proc/" element +// COMPILE ERROR /atom/movable/proc/foo() return +// This only works without the "proc" element if an override is declared /datum/var/bar = /atom/movable/foo /proc/RunTest() - return TRUE + return diff --git a/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element2.dm b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element2.dm new file mode 100644 index 0000000000..7837726ddd --- /dev/null +++ b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element2.dm @@ -0,0 +1,13 @@ +// RETURN TRUE + +/atom/movable/proc/foo() + return + +/atom/movable/foo() + return + +// This only works without the "proc" element if an override is declared +/datum/var/bar = /atom/movable/foo + +/proc/RunTest() + return TRUE diff --git a/DMCompiler/DM/Expressions/Constant.cs b/DMCompiler/DM/Expressions/Constant.cs index dcdfcdc456..8cbaf4aba1 100644 --- a/DMCompiler/DM/Expressions/Constant.cs +++ b/DMCompiler/DM/Expressions/Constant.cs @@ -385,6 +385,11 @@ bool ResolveProc(DreamPath ownerPath, string procName, bool isOverride, [NotNull if (isOverride) { procId = procs[^1]; + var dmProc = DMObjectTree.AllProcs[procId.Value]; + // Trying to resolve a procpath without the "/proc/" element only works if the proc is an override + if ((dmProc.Attributes & ProcAttributes.IsOverride) != ProcAttributes.IsOverride) { + DMCompiler.Emit(WarningCode.ItemDoesntExist, Location, $"{Value}: undefined type path"); + } } else { procId = procs[0]; if (procs.Count > 1) { From 13a87f46f5c3755ba246c13a5ef86fe0d6fbe9af Mon Sep 17 00:00:00 2001 From: ike709 Date: Tue, 1 Oct 2024 12:12:32 -0500 Subject: [PATCH 4/5] document that you can index AllProcs by procID cuz I forgor about it --- DMCompiler/DM/DMObjectTree.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/DMCompiler/DM/DMObjectTree.cs b/DMCompiler/DM/DMObjectTree.cs index 9b08ac0ad1..c0d5b087df 100644 --- a/DMCompiler/DM/DMObjectTree.cs +++ b/DMCompiler/DM/DMObjectTree.cs @@ -8,16 +8,22 @@ namespace DMCompiler.DM; internal static class DMObjectTree { public static readonly List AllObjects = new(); + + /// + /// A list of all procs. Can be indexed by a procID int to get its DMProc + /// public static readonly List AllProcs = new(); //TODO: These don't belong in the object tree public static readonly List Globals = new(); public static readonly Dictionary GlobalProcs = new(); + /// /// Used to keep track of when we see a /proc/foo() or whatever, so that duplicates or missing definitions can be discovered, /// even as GlobalProcs keeps clobbering old global proc overrides/definitions. /// public static readonly HashSet SeenGlobalProcDefinition = new(); + public static readonly List StringTable = new(); public static DMProc GlobalInitProc = default!; // Initialized by Reset() (called in the static initializer) public static readonly HashSet Resources = new(); From 8c20f22c4f2ae6d11bafaef8ed03b82795ccb201 Mon Sep 17 00:00:00 2001 From: ike709 Date: Fri, 4 Oct 2024 09:27:20 -0500 Subject: [PATCH 5/5] add test error codes --- Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm | 2 +- .../DMProject/Tests/Procs/procpath_without_proc_element.dm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm b/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm index 782ac330cf..1bc81fbc1e 100644 --- a/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm +++ b/Content.Tests/DMProject/Tests/Procs/ambiguous_procpath_error.dm @@ -1,4 +1,4 @@ -// COMPILE ERROR +// COMPILE ERROR OD2601 #pragma AmbiguousProcPath error /datum/proc/foo() diff --git a/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm index 9ab0465949..4ca3eb1465 100644 --- a/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm +++ b/Content.Tests/DMProject/Tests/Procs/procpath_without_proc_element.dm @@ -1,4 +1,4 @@ -// COMPILE ERROR +// COMPILE ERROR OD0404 /atom/movable/proc/foo() return