Change array type syntax to Array<u8, 100>

[Maltby]

What was wrong?

#273

How was it fixed?

To-Do

• OPTIONAL: Update Spec if applicable

• Add entry to the release notes (may forgo for trivial changes)

• Clean up commit history

[sbillig]

Trying to figure out how to get the CI checks to run. Ignore the "ready for review" toggle I just flipped.

[sbillig]

I can't figure it out. GH actions are set to not run automatically on PRs from first-time contributors, but normally there's a button to click to run the actions. Maybe it's not showing because of the unresolved conflict? BTW, the CI checks will reject code that's not formatted with cargo fmt --all or has clippy warnings (cargo clippy --workspace --all-targets --all-features; the --all-features flag there will built solc, which will require some libraries to be installed locally (blah); you can probably leave that flag off for now).

[g-r-a-n-t]

Maybe it's not showing because of the unresolved conflict?

Yeah, the build won't run for PRs with conflicts.

[sbillig]

I just noticed a relevant bit of code that you may or may not have discovered yet, in fn expr_call_type_constructor:

fe/crates/analyzer/src/traversal/expressions.rs

Lines 979 to 981 in e9c1ee1

	// The current array type syntax is parsed as an index operation in this case (eg `u8[10](5)`),
	// and won't end up here.
	Type::Array(_) => unreachable!(),

With the new array syntax in place, code like this will now panic:

let x: Array<u8, 3> = Array<u8, 3>([1, 2, 3])

I don't really care whether we support this right now (the Array<u8, 3> type constructor call would be redundant in this case anyway, as the type of [1, 2, 3] will be inferred), but it should at least log an error instead of panicking. We could maybe support other cases, like Array<u8, 4>([1, 2]) # pads with zeros or Array<u64, 4>(array_of_u32) or something, but maybe that sort of thing should just be a function on the array type instead of a special case of the type constructor.

[Maltby]

@sbillig I resolved the issues you noted, squashed my commits, and rebased onto master.

For the Array type constructor case in fn expr_call_type_constructor, I just added error logging for now. I'd be happy to add support for the Array type constructor in this PR or another one if you want.

[sbillig]

@Maltby looks great! The failing features::test_method_return::case_029 test has revealed a small weakness in the old-array-syntax detecting code.

error: Outdated array syntax
  ┌─ features/return_from_memory_i8.fe:6:24
  │
6 │         let in_memory: i8[2] = [val, val]
  │                        ^^^^^
  │
  = Hint: Use `Array<i8, 2>`

error: type mismatch
  ┌─ features/return_from_memory_i8.fe:6:32
  │
6 │         let in_memory: i8[2] = [val, val]
  │                                ^^^^^^^^^^ this has type `Array<i8, 2>`; expected type `i8`

error: `i8` type is not subscriptable
   ┌─ features/return_from_memory_i8.fe:12:26
   │
12 │         let result: i8 = in_memory[0] + in_memory[1]
   │                          ^^^^^^^^^ unsubscriptable type
   │
   = Note: Only arrays and maps are subscriptable

Some background: if the parser emits an error but doesn't fail (which it should do if it's able to construct a valid ast despite the error), then the analyzer will run on the resulting ast. The analyzer is producing the type errors above.

So, there are two ways to solve this:

1. just make the old-array-syntax a fatal error (by returning Err(ParseFailed)), or, slightly fancier:
2. produce a correct TypeDesc generic array node for the old syntax. To do this successfully, you'd have to change the if par.peek() == Some(BracketOpen) back to while par.peek() ..., so nested old-syntax-arrays are handled correctly, eg u8[4][4][4]

I'd be fine with option 1. Sometime in the future, when the old array syntax is sufficiently old, we may just delete the code that tries to detect it anyway. (Same goes for the code that checks for the old def keyword and suggests fn). Option 2 would result in nicer user experience, because when the parser fatally fails, no further errors can be reported, because the whole thing just aborts. Totally up to you and your appetite for continuing to tweak this. Either way, would you a parser error test or two to test this new error handling code (parser/tests/cases/errors.rs)?

Regarding the "lint" failure, that's a weakness in our docs/validate_doc_examples.py script. The failure is happening in a doc example that should be skipped, but the skipping logic doesn't work if the example code is changed.

Change this line

fe/docs/validate_doc_examples.py

Line 26 in d010ada

'clone_function.md_641905887e4885c4fe0e64666dc50089182e6c5b.fe',

to clone_function.md_b5723756c9f8ecc9364a6a871a161dcb4607f2a3.fe and that error will be cleared up. Someone can make this more robust in a future PR.

[Maltby]

@sbillig I think I'll give option 2 a whirl, seems doable enough. I'll take care of that, add some tests, fix the lint failure, and let you know when that's all done. Thanks for all your help with this!

[sbillig]

@Maltby Looks great! I pushed a couple commits (--forced) to your branch:

• made validate_doc_examples.py's skip list a bit smarter (turns out a few other files needed their hashes updated as well; the hashes have been replaced with simple per-file indexes)
• added a "newsfragment" file for the release notes, and fixed a minor build issue due to the latest change to the master branch

Thanks for doing all of this! I'll merge when the checks finish running.

[codecov-commenter]

Codecov Report

Merging #571 (7ef7c21) into master (674b49b) will decrease coverage by 0.08%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
- Coverage   88.25%   88.17%   -0.09%     
==========================================
  Files          87       87              
  Lines        7076     7101      +25     
==========================================
+ Hits         6245     6261      +16     
- Misses        831      840       +9     

Impacted Files	Coverage Δ
crates/abi/src/builder.rs	97.33% <ø> (ø)
crates/analyzer/src/traversal/expressions.rs	89.07% <0.00%> (-0.22%)	⬇️
crates/analyzer/src/traversal/types.rs	83.87% <ø> (-0.98%)	⬇️
crates/lowering/src/mappers/module.rs	83.11% <0.00%> (ø)
crates/lowering/src/mappers/types.rs	100.00% <ø> (ø)
crates/parser/src/ast.rs	87.61% <ø> (-0.04%)	⬇️
crates/lowering/src/names.rs	66.66% <37.50%> (-11.91%)	⬇️
crates/analyzer/src/namespace/types.rs	76.47% <86.66%> (-0.26%)	⬇️
crates/parser/src/grammar/types.rs	88.26% <100.00%> (+2.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 674b49b...7ef7c21. Read the comment docs.