onflow / flips

Flow Improvement Proposals
24 stars 22 forks source link

Add flip for changing import statement semantics #277

Closed SupunS closed 1 week ago

SupunS commented 3 weeks ago

Work towards https://github.com/onflow/flips/issues/278

bluesign commented 2 weeks ago

What are the breaking examples for this on staged contracts ? I can think of access(account) var cases as most critical ( will require setter I suppose in the current model ) , structs as references can be pretty easy to fix.

SupunS commented 2 weeks ago

@bluesign Like mentioned in the impact section, there were three contracts affected by this, and all of them were returning an imported-contract field (a dictionary) as-is, so the type annotation of the function would needed to be changed. e.g: similar to https://github.com/onflow/flips/pull/277#discussion_r1643013008. I don't quite know what the use-case of those functions are; They were not used anywhere AFAIU (did a string search in all staged contracts, but came empty).

turbolent commented 2 weeks ago

I can think of access(account) var cases as most critical ( will require setter I suppose in the current model )

Yeah, that's one of the downsides, but IMHO probably not a big deal, given the workaround is adding a setter. Adding special rules to the language for treating access(account) differently is not worth it (more rules increase the burden for users and implementors, and there is little benefit over an explicit setter function)

bluesign commented 2 weeks ago

Yeah, that's one of the downsides, but IMHO probably not a big deal, given the workaround is adding a setter.

Yeah I checked mainnet contracts now, it seems it is rarely used to modify, often used to just read.

bjartek commented 2 weeks ago

LGTM, this is a no brainer.

austinkline commented 2 weeks ago

Proposal is pretty straight-forward, a bit surprised it didn't work this way already!

:+1:

bluesign commented 2 weeks ago

LGTM

SupunS commented 2 weeks ago

For additional information, these are the fixes needed for the three breaking contracts: https://gist.github.com/SupunS/c732360df3fa969e09a161776a1af0ab

SupunS commented 2 weeks ago

We discussed this FLIP during today's developer office hours / working group call, and the sentiment was very positive, and there have not been any concerns so far. If it continues to have no any further concerns, we will consider this FLIP as approved by the EOW.

SupunS commented 1 week ago

Thanks everyone for the feedback! This FLIP has been open for a while, and had multiple breakout sessions for discussing the details. There has been no concerns and the feedback has been very positive. Thus, I'll consider this FLIP as approved! 🎉

You can follow the implementation progress at https://github.com/onflow/cadence/pull/3417