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

compiler: set default values for complex struct fields (2.x) #952

Merged
merged 3 commits into from
Jun 11, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented May 14, 2020

Closes #949 .

There is no way we can distinguish []int(nil) from []int{} without substantial effort. And even then array is a reference type so it's instance is equal only to itself in NeoVM.
There is a simple workaround for checking if slice is empty: len(arr) == 0.

  1. Emit error on comparison with nil
  2. Fix type dispatch during != processing.

@roman-khimov
Copy link
Member

Can't we use a false boolean for nil representation?

@fyrchik
Copy link
Contributor Author

fyrchik commented May 14, 2020

Yes, this is one way to fix this, however each such fix requires proper type conversions in other array related opcodes (e.g. emitting proper JMP in case we encounter bool before APPEND).
This is too much work for a single rare corner case that has workaround IMO. We can fail compiling though.

@roman-khimov
Copy link
Member

If we're not supporting it then failing to compile is an option. I understand that it's easy to fix in Neo 3 because of native NULL type, but here we should either fix it or fail compilation. I'd probably opt for fixing it as nil is not something obscure for Go and I don't think we have a lot of points where this would require adjusting (cases where doing something with nil thing is valid).

@alexvanin
Copy link
Contributor

I noticed in v0.75.0 that structure as interface{} value can be nil checked with == operator, but it crashes in runtime with != operator when interface{} is not nil.

package foo
type bar struct {
	x int
}
func foo(i int) interface{} {
	if i > 0 {
		return bar{x: i}
	}
	return nil
}
func Main() int {
	f := foo(1)
	if f != nil {
		return 1
	}
	return 2
}

Maybe it is somehow related to solving issue.

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #952 into master-2.x will increase coverage by 0.14%.
The diff coverage is 96.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           master-2.x     #952      +/-   ##
==============================================
+ Coverage       67.74%   67.89%   +0.14%     
==============================================
  Files             146      146              
  Lines           14384    14404      +20     
==============================================
+ Hits             9744     9779      +35     
+ Misses           4177     4162      -15     
  Partials          463      463              
Impacted Files Coverage Δ
pkg/compiler/codegen.go 88.61% <95.55%> (+0.05%) ⬆️
pkg/compiler/analysis.go 96.47% <100.00%> (+0.73%) ⬆️
pkg/compiler/debug.go 87.09% <100.00%> (+0.16%) ⬆️
pkg/consensus/consensus.go 41.22% <0.00%> (+1.80%) ⬆️
pkg/consensus/block.go 94.11% <0.00%> (+16.33%) ⬆️

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 788304e...cc5b5bf. Read the comment docs.

Set default value also for complex (struct, map) types.
Note: because VM does not distinguish empty array and nil array,
comparison 'a == nil' can't be handled properly without substantial
effort. This will be fixed in NEO3.
NEO VM does not distinguish between empty and nil slices. Supporting
this is not easy and requires changing lots of other opcodes.
Pointers are not supported anyway and slices can be checked for
emptiness by inspecting their `len`.
Dispatch based on types similarly to EQUAL.
@roman-khimov roman-khimov added this to the v0.75.1 milestone Jun 11, 2020
@roman-khimov roman-khimov merged commit 9615e83 into master-2.x Jun 11, 2020
@roman-khimov roman-khimov deleted the fix/structfield branch June 11, 2020 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler Go smart contract compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants