fix: also consider references in array validation#1660
Conversation
array validators did not account for references when counting the initialised elements
| // CHECK-DAG: {{.*}}array_partial_init_warning_type_and_var.st:{{[0-9]+}}:{{[0-9]+}}:{{.*}}error[E127]: Fewer initial values for array `a` than expected. Expected 5, found 2. | ||
| // CHECK-DAG: {{.*}}array_partial_init_warning_type_and_var.st:{{[0-9]+}}:{{[0-9]+}}:{{.*}}error[E127]: Fewer initial values for array `userArrayType` than expected. Expected 5, found 4. |
There was a problem hiding this comment.
Hate this, not for the fact that we UI test validations here (big fan) but that we're UI testing with lit. I think we should discuss about moving validations to a better UI testing framework, something similar to what astral does with ruff and rustc. At least that makes sense since the introduction of lowering.
There was a problem hiding this comment.
would you say it's a blocker for this fix now? i generally agree
There was a problem hiding this comment.
I'd say no but we should write it down somewhere (either here or Jira) to convert existing validations to dedicated files with std{out,err} matching. Is bats a good framework for that btw?
There was a problem hiding this comment.
i tend to say no, it does not do string comparisons that well, and tests are written in shell language and can be hard to read. I prefer the lit way of inline assertions. So a file has both the code and its assertions
as part of the array validation improvements, make sure recursive arrays don't break
| 11 │ TYPE userArrayType : ARRAY [1..5] OF INT := [1,2,4,5]; | ||
| │ ^^^^^^^^^ Fewer initial values for array `userArrayType` than expected. Expected 5, found 4. |
There was a problem hiding this comment.
I like it! I wonder though, will this develop into a similar situation like the "[...] use REF_TO instead of POINTER TO [...]" validation which we just disabled because it got too noisy for the user? Rust solved this nicely with validations vs clippy lints imo.
Build Artifacts🪟 Windows
From workflow run 🐧 Linux
From workflow run |
2 participants
This pull request improves the handling and testing of array initialization diagnostics, specifically for cases where arrays are initialized with fewer elements than their declared size. It enhances both the validation logic and the test infrastructure to ensure that warnings about partially initialized arrays are consistently detected and can be promoted to errors for stricter testing.
Validation logic improvements:
statement_to_array_lengthfunction insrc/validation/array.rsto handleReferenceExprstatements, ensuring that the array length is determined correctly for references, defaulting to 1 if the length cannot be determined.Testing enhancements:
fewer_elements_in_type_and_derived_variableinsrc/validation/tests/array_validation_test.rsto verify that warnings (E127) are emitted when both a type and a variable are initialized with too few elements.array_partial_init_warning_type_and_var.stto check that both relevant diagnostics are emitted and promoted to errors when E127 is configured as an error.diagnostics_e127_as_error.jsonto promote warning E127 to an error for testing purposes.Test infrastructure updates:
tests/lit/lit.cfgto add the%PLCsubstitution, making it easier to invoke the compiler in lit tests.