fix: ensuring ADR assignment on REFERENCE TO variables raises an error

[Angus-Bethke-Bachmann]

Changed

• Ensuring ADR assignment on REFERENCE TO variables raises an error

Chore

• Removed some warnings in the enum validation tests

Testing

• Added validation tests to verify that ADR assignment on REFERENCE TO variables produces an error message

[ghaith]

The failing build will be fixed with the fix_arm_constructors branch, we can ignore it here

[volsa]

I briefly skimmed through this PR, that line caught my attention however. Is there perhaps an alternative to string matching? I believe the built-ins have their own closure (functions) which do validation, not sure they carry enough information to enclose the left side however. Might be worth looking into?

[Angus-Bethke-Bachmann]

I had a look at the builtins, but we have no helper functions that specifically validate the function name. I could however adapt this to call the "get_builtin" method as an extra step of validation to verify that that the call statement is a builtin before directly verifying that it is the ADR function? In either case I will need to do string validation.

Perhaps then it makes sense to make a helper function in the builtins for this specific purpose? Let me know what you think.

[volsa]

Perhaps then it makes sense to make a helper function in the builtins for this specific purpose?

If you can think of a way that is not overkill go for it, otherwise let's move forward with the pragmatic (though maybe not performative) solution of raw string matching. fwiw, opus recommended

   fn node_is_builtin_adr<T: AnnotationMap>(node: &AstNode, context: &ValidationContext<T>) -> bool {
       let AstStatement::CallStatement(CallStatement { operator, .. }) = node.get_stmt_peeled() else {
           return false;
       };

       let Some(call_name) = context.annotations.get_call_name(operator.as_ref()) else {
           return false;
       };

       let Some(adr_builtin) = builtins::get_builtin("ADR") else {
           return false;
       };

       context
           .index
           .get_builtin_function(call_name)
           .is_some_and(|builtin| std::ptr::eq(builtin, adr_builtin))
   }

(essentially what you mentioned in your comment)

[Angus-Bethke-Bachmann]

I have implemented this suggestion :)

[volsa]

I'm assuming this aligns with the behavior of CodeSys?

[Angus-Bethke-Bachmann]

The CODESYS behaviour is slightly different, it returns a type mismatch: "Cannot convert type 'POINTER TO DINT' to type 'DINT'".

So we arrive at the same conclusion via different means. In this case, I believe we give more information about the problem and hint at resolving it. That being said, I am open to trying to resolve this as a type mismatch validation.

[volsa]

Nope, I was just curious whether ADR is truly disallowed in a REFERENCE TO context. I agree though, our message in that case is much more helpful.

[codecov]

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.65%. Comparing base (4e9592b) to head (06a660d).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/validation/statement.rs	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1597      +/-   ##
==========================================
- Coverage   94.66%   94.65%   -0.02%     
==========================================
  Files         185      187       +2     
  Lines       58437    58840     +403     
==========================================
+ Hits        55322    55695     +373     
- Misses       3115     3145      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.