Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce dereference on the Reference type #2945

Conversation

darkdrag00nv2
Copy link
Contributor

Closes #2803

Description


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Copy link
Contributor Author

@darkdrag00nv2 darkdrag00nv2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that there are two types of reference values - EphemeralReferenceValue and StorageReferenceValue.

Should this function be implemented on both or just EphemeralReferenceValue?

sema.ReferenceDereferenceFunctionType(v.BorrowedType),
func(invocation Invocation) Value {
return v.Value
},
Copy link
Contributor Author

@darkdrag00nv2 darkdrag00nv2 Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of dereference on EphemeralReferenceValue looks straightforward. Is this correct or am I missing something?

Copy link
Contributor

@bluesign bluesign Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this create new Value (copy of the value somehow ) ? something like Transfer etc.

This comment was marked as outdated.

Copy link
Contributor

@bluesign bluesign Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more like:

let x: &[Int] = &[1]
x.dereference().append(2)

PS: if we make a copy also we can support non-primitives like structs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this needs a Transfer to the stack, i.e.

.Transfer(
	interpreter,
	locationRange,
	atree.Address{},
	false,
	nil,
	nil,
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point @bluesign to test by operating on the returned value directly, my example above, let ints2 = ref.dereference() was adding a copy 👍

@turbolent
Copy link
Member

@darkdrag00nv2 yeah, dereferencing needs to be implemented for both EphemeralReferenceValue and StorageReferenceValue.

Dereferencing should be supported at the type level for references in general (&T), for all primitive types (e.g. Int -> &Int), or containers of primitive types (e.g. [Int] -> &[Int]). At the value level there are two different kinds of values that both represent values of reference types. Basically, let v: &T might either hold be a EphemeralReferenceValue or a StorageReferenceValue at run-time.

@darkdrag00nv2
Copy link
Contributor Author

@turbolent You've mentioned primitive types and containers of primitive types.

Could we not define it for all subtypes of AnyStruct? Basically ignoring all resource types. Do you see any complications/extra-work to support them, say for Struct?

@bluesign
Copy link
Contributor

bluesign commented Nov 17, 2023

Could we not define it for all subtypes of AnyStruct? Basically ignoring all resource types. Do you see any complications/extra-work to support them, say for Struct?

Only edge case I see is attachments ( Contracts are covered by transfer ), attachments would allow detaching and attaching to new struct. ( which we don't allow currently )

struct T{
  access(all) var tag : Int
  init(_ tag:Int){
     self.tag = tag
  }
}
						
attachment X for AnyStruct{

}

fun main(): AnyStruct {
   var t1 = T(1)
   var t2 = T(2)
   var res = attach X() to t1

   var ff = res[X]!.dereference
   var newAttached = attach ff() to t2
   return newAttached
}

@SupunS SupunS self-assigned this Nov 21, 2023
@SupunS SupunS added the Feature label Nov 21, 2023
@dsainati1 dsainati1 deleted the branch onflow:feature/stable-cadence December 11, 2023 14:57
@dsainati1 dsainati1 closed this Dec 11, 2023
@SupunS
Copy link
Member

SupunS commented Dec 11, 2023

Not sure why this got closed. This should now be opened against the master (#2971)

@dsainati1
Copy link
Contributor

Same, it seems as though it was closed automatically when feature/stable-cadence was merged into master. I can't re-open it for some reason though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants