Fix cyclic dependencies bug #1180
Conversation
There was a problem hiding this comment.
I'm not sure if this is the correct fix for this bug.
Pkl allows circular imports (except in cases where a module's type is defined in terms of itself, like two modules extending each other).
Also, amending/extending modules expose types defined on their parent.
I created a reproducer for this bug in this issue here: #1183
I think either:
- We throw a meaningful error about some circular dependency
- We fix some deeper underlying issue so that this code works
BTW: can we add the reproducer as a test case here? You can add them to the input-helpers dir.
|
|
||
| var importedModule = (VmTyped) result; | ||
| if (importedModule.isNotInitialized() | ||
| && importedModule.isModuleObject() |
There was a problem hiding this comment.
This should always be a module object, right?
There was a problem hiding this comment.
Indeed. Removed the check.
| } | ||
| } | ||
|
|
||
| module.setCachedValue(importName, result); |
There was a problem hiding this comment.
We shouldn't be cacheing this under the same name as the import value. Otherwise, this snippet breaks:
import "amendingFoo.pkl"
// here, `amendingFoo` should resolve to the prototype
res: amendingFoo.Foo
// here, `amendingFoo` should resolve to the import itself
bar = amendingFooThere was a problem hiding this comment.
Good catch. cycles2.pkl tests that (fails if you cache).
| var amendedModuleKey = moduleInfo.getAmendedModuleKey(); | ||
|
|
||
| if (amendedModuleKey != null) { | ||
| return VmLanguage.get(null).loadModule(amendedModuleKey); |
There was a problem hiding this comment.
The amended module key itself may be amending another module; we need to keep going until we find the first non-amending module. Can we add a test for this?
Also, pass the node in when looking up the reference
| return VmLanguage.get(null).loadModule(amendedModuleKey); | |
| return VmLanguage.get(this).loadModule(amendedModuleKey, this); |
There was a problem hiding this comment.
Indeed! I changed the test to have another layer of indirection.
| if (prototypeModule != null) { | ||
| result = prototypeModule; | ||
| } | ||
| } |
There was a problem hiding this comment.
The result of getImport should always be the prototype in all cases, even when the module has not yet been initialized.
pkl-core/src/main/java/org/pkl/core/ast/type/ResolveDeclaredTypeNode.java
Show resolved
Hide resolved
In some very specific scenarios cyclic dependencies may lead to a null pointer due to the module not being properly initialized.
Fixes #1183.