self declarations

[g-r-a-n-t]

What was wrong?

self was implicitly defined in all function scopes, which made it impossible to write pure functions.

How was it fixed?

1.) added self args to the function definition grammar
2.) added checking around the use of self
3.) fixed things that broke

I also went ahead and updated the Json ABI builder to label functions that do not take a self parameter as pure. This is something that I'm not sure we want right now though, as functions can still read msg and chain data (which conflicts with the definition of pure). For this reason, I'm considering dropping the commit responsible for this change, but have not 100% made up my mind. Feel free to provide input.

To-Do

• Update Spec if applicable
• Add entry to the release notes (may forgo for trivial changes)
• Clean up commit history
• Update grammar

[codecov-commenter]

Codecov Report

Merging #520 (712fb60) into master (6fa4753) will increase coverage by 0.13%.
The diff coverage is 92.36%.

❗ Current head 712fb60 differs from pull request most recent head 5913da1. Consider uploading reports for the commit 5913da1 to get more accurate results

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   87.55%   87.68%   +0.13%     
==========================================
  Files          87       87              
  Lines        5585     5670      +85     
==========================================
+ Hits         4890     4972      +82     
- Misses        695      698       +3     

Impacted Files	Coverage Δ
crates/analyzer/src/context.rs	87.80% <ø> (ø)
crates/analyzer/src/namespace/types.rs	72.87% <ø> (ø)
crates/lowering/src/mappers/expressions.rs	100.00% <ø> (ø)
crates/parser/src/grammar/functions.rs	90.87% <78.57%> (+0.06%)	⬆️
crates/analyzer/src/db/queries/functions.rs	91.95% <89.65%> (+1.16%)	⬆️
crates/analyzer/src/traversal/expressions.rs	90.54% <90.69%> (+0.35%)	⬆️
crates/analyzer/src/db.rs	100.00% <100.00%> (ø)
crates/analyzer/src/db/queries/contracts.rs	97.63% <100.00%> (+0.15%)	⬆️
crates/analyzer/src/namespace/items.rs	91.16% <100.00%> (+0.51%)	⬆️
crates/analyzer/src/namespace/scopes.rs	82.85% <100.00%> (-1.31%)	⬇️
... and 6 more

---

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 6fa4753...5913da1. Read the comment docs.

[g-r-a-n-t]

@cburgdorf I saw your comment about using Self::pure_func(), but it appears to have been wiped in a rebase 🤔.

This is what I was thinking too.

[cburgdorf]

Nice work! Left a few comments inline. There's a regression introduced that should be addressed.

Regarding:

I also went ahead and updated the Json ABI builder to label functions that do not take a self parameter as pure

I think we shouldn't mark them as pure in the ABI then imho. We could still mark it as view though even if it is technically a bit purer than view :)

I kinda dislike the current syntax of calling pure functions on the contract by just their name (e.g. foo()). You seem to be on board with demanding those to be prefixed with Self:: (e.g. Self::foo()) but we should probably create an issue for that. Also paging in @sbillig for visibility. It's also not just an aesthetically question since the current syntax would become ambiguous with imported functions or functions that are defined outside of the contract but within the same file.

Since today is the last day of August and we should cut our monthly release, I'm a bit hesitant if we just want to leave this PR for the next release so that we should be able to bundle it with the Self:: change?

[sbillig]

I kinda dislike the current syntax of calling pure functions on the contract by just their name (e.g. foo()).

I'm not sure how I feel about this; it seems like Self:: would would be unnecessary noise here. If fn foo() is defined in the current contract scope, why require the scope to be specified? We aren't going to require a scope prefix for functions defined at the module level and called within the same module; why should functions defined in a contract be treated differently?

It's also not just an aesthetically question since the current syntax would become ambiguous with imported functions or functions that are defined outside of the contract but within the same file.

The analyzer can check for ambiguity, and require the user to disambiguate.

[sbillig]

Looks great. I guess I agree non-self fns should be called views until we can be sure that they don't access state (but also think that it doesn't really matter at this stage of development).

[cburgdorf]

The analyzer can check for ambiguity, and require the user to disambiguate.

That's true but I kinda like the added clarity of avoiding the ambiguity in the first place :)