Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a major architectural refactoring to enable support for multiple concurrent weaver instances by removing the thread-local weaver pattern and introducing explicit weaver references throughout the codebase.
Changes:
- Removed thread-local
WeaverEnginestorage, requiring join points to receive aWeaverEnginereference via constructor - Updated code generation in
WeaverGeneratorto produce join point constructors that accept weaver references - Removed deprecated JavaScript APIs (
Ast.ts,Weaver.serialize,Weaver.deserialize) and the--versionCLI option - Updated GitHub workflows to improve branch resolution logic for dependency repositories
- Updated multiple npm package dependencies in Lara-JS
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| WeaverInterface/src/org/lara/interpreter/weaver/interf/WeaverEngine.java | Removed thread-local weaver storage (getThreadLocalWeaver, setWeaver, removeWeaver, isWeaverSet) and getNameAndBuild() method |
| WeaverInterface/src/org/lara/interpreter/weaver/interf/JoinPoint.java | Added weaver field and constructor parameter to store weaver reference per join point instance |
| WeaverInterface/src/org/lara/interpreter/weaver/ast/AAstMethods.java | Added getWeaverEngine() accessor method |
| WeaverGenerator/src/org/lara/interpreter/weaver/generator/generator/java/helpers/*.java | Updated code generators to include weaver constructor parameters in generated join point classes |
| WeaverGenerator/test-resources/golden//.java.txt | Updated golden test files reflecting new constructor patterns with weaver parameters |
| WeaverGenerator/test-resources/spec/valid/medium/actionModel.xml | Added return type (joinpoint[]) to the insert action |
| WeaverInterface/test/**/*.java | Updated all tests to pass weaver references when constructing join points |
| DefaultWeaver/src/org/lara/interpreter/weaver/defaultweaver/**/*.java | Updated join point implementations to accept and pass weaver references |
| Lara-JS/src-code/Weaver.ts | Removed setWeaver() call and stored weaver reference in globalThis.__hidden |
| Lara-JS/src-api/weaver/Weaver.ts | Changed getWeaverEngine() to access weaver from globalThis.__hidden, removed serialize, deserialize, and AST_METHODS |
| Lara-JS/src-api/weaver/Ast.ts | Deleted entire deprecated AST utility class |
| Lara-JS/src-api/core.ts | Deleted auto-import initialization file |
| Lara-JS/src-api/lara/benchmark/BenchmarkInstance.ts | Replaced AST caching with error throw since serialize/deserialize were removed |
| Lara-JS/package.json | Updated multiple npm dependencies to newer versions |
| Lara-JS/eslint.config.js | Fixed tsconfigRootDir placement and added __dirname setup for ES modules |
| LARAI/src/org/lara/interpreter/cli/CLIOption.java | Removed version CLI option |
| LARAI/src/org/lara/interpreter/cli/*.java | Removed version option handling from parsers and interfaces |
| LARAI/src/org/lara/interpreter/utils/LaraIUtils.java | Removed version printing logic from printHelp() |
| LARAI/test/**/*.java | Removed tests for version option functionality |
| .github/workflows/nightly.yml | Improved branch resolution logic with priority-based ref determination |
| .github/workflows/copilot-setup-steps.yml | Applied same branch resolution improvements as nightly workflow |
| .github/workflows/ant-lara-2.0-legacy.yml | Changed cron schedule from daily to monthly, updated checkout action to v6 |
| .github/copilot-instructions.md | Removed reference to deleted core.ts file |
| console.log(`Saving AST to file ${cachedFile.getAbsolutePath()}...`); | ||
| const serialized = Weaver.serialize(Query.root()); | ||
| Io.writeFile(cachedFile, serialized); | ||
| throw new Error("BenchmarkInstance.load: Caching not implemented"); |
There was a problem hiding this comment.
The error message states "Caching not implemented" but doesn't provide guidance on what users should do instead. Since the serialize/deserialize methods were removed, consider providing more context about alternative approaches or whether caching will be re-implemented in the future.
| throw new Error("BenchmarkInstance.load: Caching not implemented"); | |
| throw new Error( | |
| "BenchmarkInstance.load: AST caching is currently not implemented. " + | |
| "Please keep BenchmarkInstance.CACHE_ENABLE set to false and rely on reparsing the benchmark instead. " + | |
| "Caching support was removed with the previous serialize/deserialize API and may be reintroduced in a future version." | |
| ); |
WeaverInterface/test/org/lara/interpreter/weaver/interf/CoreClassesSmokeTest.java
Show resolved
Hide resolved
| (globalThis as any).__hidden = {javaWeaver: Weaver.javaWeaver}; | ||
|
|
There was a problem hiding this comment.
The global __hidden object is being used to store the Java weaver instance. This approach is fragile and could lead to naming conflicts or security issues. Consider using a module-scoped variable or a WeakMap-based approach instead of polluting the global namespace.
| (globalThis as any).__hidden = {javaWeaver: Weaver.javaWeaver}; |
| tsconfigRootDir: __dirname, | ||
|
|
||
| parser: typescriptEslint.parser, | ||
| ecmaVersion: 5, |
There was a problem hiding this comment.
The ESLint configuration is using ecmaVersion: 5 which is very old (ES5/2009). This should likely be updated to a modern ECMAScript version (e.g., 2020 or later) to match the TypeScript output and Node.js 20+ requirements specified in package.json.
| ecmaVersion: 5, | |
| ecmaVersion: 2020, |
…ad-local weaver references
Root Cause: The test was calling WeaverGenerator.main(args) which internally calls System.exit(), terminating the JVM before the golden file comparison could execute. This is why the changes to WeaverGenerator weren't being detected - the comparison code was never reached. Fix Applied: Changed both calls from WeaverGenerator.main(args) to WeaverGenerator.run(args) with proper exit code assertions. Result: The tests now run to completion and properly compare generated files against golden files.
…the past commits. - Removed the version option from CLIOption and related parsing logic. - Updated OptionsParser to remove references to the version option. - Adjusted help printing methods to exclude version information. - Modified tests to reflect the removal of the version option, ensuring no references remain in CLIOptionTest and OptionsBuilderUtilsTest. - Updated various tests to use the new TestJoinPoint constructor that requires a WeaverEngine instance. - Cleaned up WeaverEngine and JoinPoint tests to remove unnecessary thread-local weaver management. - Ensured all tests are consistent with the new structure and functionality.
6d36bb8 to
67fed4b
Compare
No description provided.