Skip to content

Conversation

@lm-sousa
Copy link
Member

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 WeaverEngine storage, requiring join points to receive a WeaverEngine reference via constructor
  • Updated code generation in WeaverGenerator to produce join point constructors that accept weaver references
  • Removed deprecated JavaScript APIs (Ast.ts, Weaver.serialize, Weaver.deserialize) and the --version CLI 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");
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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."
);

Copilot uses AI. Check for mistakes.
Comment on lines +153 to +154
(globalThis as any).__hidden = {javaWeaver: Weaver.javaWeaver};

Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
(globalThis as any).__hidden = {javaWeaver: Weaver.javaWeaver};

Copilot uses AI. Check for mistakes.
tsconfigRootDir: __dirname,

parser: typescriptEslint.parser,
ecmaVersion: 5,
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
ecmaVersion: 5,
ecmaVersion: 2020,

Copilot uses AI. Check for mistakes.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants