Allow resolving method calls to methods declared in hierarchical parents#1688
Allow resolving method calls to methods declared in hierarchical parents#1688mampcs wants to merge 1 commit intoMikePopoloski:masterfrom
Conversation
|
Man, this is cursed. It seems like all commercial tools allow this though -- I can kind of see how they could interpret this as the rule from 23.8 and 23.8.1, even though I don't really agree. It probably makes sense for slang to allow this without a flag though, since all other tools allow it. |
source/ast/Expression.cpp
Outdated
| // Normal name lookup and resolution. | ||
| LookupResult result; | ||
| Lookup::name(syntax, context, flags, result); | ||
|
|
There was a problem hiding this comment.
I think instead of doing another lookup, you can instead check for this prior to doing the lookup and set the AlwaysAllowUpward flag.
There was a problem hiding this comment.
That's what i tried first :-)
But that leads to other issues. Let me add it back and detail the error that it was leading to. I believe it was causing one of the unit tests to fail where there was a class method declared outside the the class using templates.
There was a problem hiding this comment.
Making the following code change resulted in multiple unit test failures:
@@ -1006,9 +1006,11 @@ Expression& Expression::bindName(Compilation& comp, const NameSyntax& syntax,
const ArrayOrRandomizeMethodExpressionSyntax* withClause,
const ASTContext& context) {
bitmask<LookupFlags> flags = LookupFlags::None;
- if ((invocation && invocation->arguments) ||
- comp.hasFlag(CompilationFlags::AllowUseBeforeDeclare)) {
- flags |= LookupFlags::AllowDeclaredAfter;
+ if (invocation && invocation->arguments) {
+ if (comp.hasFlag(CompilationFlags::AllowUseBeforeDeclare)) {
+ flags |= LookupFlags::AllowDeclaredAfter;
+ }
+ flags |= LookupFlags::AlwaysAllowUpward;
}
Unit test failures:
-------------------------------------------------------------------------------
Subroutine lookup
-------------------------------------------------------------------------------
/.../slang/tests/unittests/ast/LookupTests.cpp:452
...............................................................................
/.../slang/tests/unittests/ast/LookupTests.cpp:471: FAILED:
explicitly with message:
source:7:20: error: expression is not callable
int b = foo();
~~~^
/.../slang/tests/unittests/ast/LookupTests.cpp:476: FAILED:
CHECK( block.find<VariableSymbol>("b").getInitializer()->eval(context).integer() == 4 )
due to unexpected exception with message:
std::get: wrong index for variant
-------------------------------------------------------------------------------
Spurious inheritance from generic param error -- GH #1058
-------------------------------------------------------------------------------
/.../slang/tests/unittests/ast/ClassTests.cpp:3356
...............................................................................
/.../slang/tests/unittests/ast/ClassTests.cpp:3405: FAILED:
explicitly with message:
source:37:8: error: use of undeclared identifier 'get_name'
if(get_name() == "string") begin
^~~~~~~~
-------------------------------------------------------------------------------
Driver full compilation with defines and param overrides
-------------------------------------------------------------------------------
/.../slang/tests/unittests/DriverTests.cpp:389
...............................................................................
/.../slang/tests/unittests/DriverTests.cpp:401: FAILED:
CHECK( driver.runFullCompilation() )
with expansion:
false
/.../slang/tests/unittests/DriverTests.cpp:402: FAILED:
CHECK( stdoutContains("Build succeeded") )
with expansion:
false
-------------------------------------------------------------------------------
Used-before-declared flag with parameters
-------------------------------------------------------------------------------
/.../slang/tests/unittests/ast/LookupTests.cpp:2595
...............................................................................
/.../slang/tests/unittests/ast/LookupTests.cpp:2636: FAILED:
explicitly with message:
source:4:16: error: identifier 'WIDTH' used before its declaration
output wire [WIDTH-1:0] rd, // use before declaration
^~~~~
source:11:13: note: declared here
parameter WIDTH = 16;
^
source:7:15: error: identifier 'WIDTH' used before its declaration
input wire [WIDTH-1:0] wd // use before declaration
^~~~~
source:11:13: note: declared here
parameter WIDTH = 16;
^
source:29:23: error: identifier 'COLS' used before its declaration
input [7+$clog2(COLS):0] A;
^~~~
source:32:16: note: declared here
localparam COLS=4;
There was a problem hiding this comment.
That's because your patch disables AllowUseBeforeDeclare for non-subroutines. The correct patch looks like:
--- a/source/ast/Expression.cpp
+++ b/source/ast/Expression.cpp
@@ -1006,10 +1006,11 @@ Expression& Expression::bindName(Compilation& comp, const NameSyntax& syntax,
const ArrayOrRandomizeMethodExpressionSyntax* withClause,
const ASTContext& context) {
bitmask<LookupFlags> flags = LookupFlags::None;
- if ((invocation && invocation->arguments) ||
- comp.hasFlag(CompilationFlags::AllowUseBeforeDeclare)) {
+ if (invocation && invocation->arguments)
+ flags |= LookupFlags::AllowDeclaredAfter | LookupFlags::AlwaysAllowUpward;
+
+ if (comp.hasFlag(CompilationFlags::AllowUseBeforeDeclare))
flags |= LookupFlags::AllowDeclaredAfter;
- }
Then there's only one failing test, Spurious inheritance from generic param error -- GH #1058, which is actually an "issue" with the AlwaysAllowUpward lookup handling. I think this patch is the right fix for that:
--- a/source/ast/Lookup.cpp
+++ b/source/ast/Lookup.cpp
@@ -1170,8 +1170,12 @@ void Lookup::name(const NameSyntax& syntax, const ASTContext& context, bitmask<L
if (!result.found) {
if (flags.has(LookupFlags::AlwaysAllowUpward)) {
+ LookupResult originalResult = result;
if (!lookupUpward({}, name, context, flags, result))
return;
+
+ if (!result.found)
+ result = originalResult;
}
There was a problem hiding this comment.
Sorry my mistake. Your first patch is kind of what i had earlier and it was leading to that exact same failure (Spurious inheritance from generic param error). Let me take a look as your second patch.
There was a problem hiding this comment.
Basically the saved LookupResult contains the flag indicating that we found a class with an error in its base class resolution, which prevents us from issuing a follow-on error. If upward lookup fails to find anything we want to grab that original result back so we don't lose that flag.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1688 +/- ##
==========================================
- Coverage 94.54% 94.53% -0.01%
==========================================
Files 235 235
Lines 53764 53769 +5
==========================================
+ Hits 50830 50833 +3
- Misses 2934 2936 +2
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Major vendors allows a child module instance to refer to a method declared in a parent module (as long as all parents of that child module have a definition of that method).
2 participants
VCS allows a child module instance to refer to a method declared in a parent module (as long as all parents of that child module have a definition of that method).