Add flip for changing import statement semantics

[SupunS]

Work towards #278

[dsainati1]

This is obviously beyond the scope of this current FLIP (and would be a new feature that can be added after Cadence 1.0), but I think that changing imports to create an unentitled reference to the contract value does suggest that it may be useful to also be able to import an entitled reference to the contract as well. There would need to be some kind of way to guarantee that only certain people can do this, and syntax for it as well.

[bluesign]

I think easiest solution is to disallow entitlements on contract functions. ( which can be relaxed later if needed )

[turbolent]

I double checked this, and entitled contract members are already forbidden: https://github.com/onflow/cadence/blob/a1199d656c222a1253fdb362c20bd05f7c310287/runtime/tests/checker/entitlements_test.go#L1673-L1686

[turbolent]

LGTM! Just a few minor suggestions for phrasing

[bjartek]

So code that would need to be changed here is if you directly use a imported contracts fields and mutate them right?

[SupunS]

yes, but in addition, if you use a field of a imported inside a function (say for example return as is), then the type annotation would change.
e.g:

fun someFunction(): T {
    return ImportedContract.foo
}

then this would change to

fun someFunction(): &T {  // <-- reference type
    return ImportedContract.foo
}

(This is also the change needed for those three contracts that breaks)

[bjartek]

Personally I think this is very dangerous. Cadence-1.0 is already breaking so this is just one more thing that we need to do.

[SupunS]

Agree!

[bluesign]

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]

@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 #277 (comment). 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]

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]

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]

LGTM, this is a no brainer.

[austinkline]

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

👍

[bluesign]

LGTM

[SupunS]

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

[SupunS]

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]

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 onflow/cadence#3417