fix[ux]: remove side effects in compare_type for bytestrings

[tserg]

What I did

Removed side effects in _Bytestring.compare_type().

How I did it

The implementation of compare_type for bytestrings are now simplified to match the top-level definition of compare_type: compare_type should have the meaning: "an expr of type <other> can be assigned to an expr of type <self>.".

For interface functions that return bytestring types and are imported via JSON, the bytestring types are initialized to zero. To make these functions identifiable, a returns_abi_bytestring attribute is added to FunctionDef types. Now, when these JSON-imported functions are called and the return value has a concrete bytestring type with a defined length, we create a new copy of the function type and overwrite the return type of the function type during annotation in visit_Call. This lets different calls of the same interface function have different lengths for the same bytestring type.

How to verify it

Commit message

fix: remove side effects in compare_type for bytestrings

Description for the changelog

Remove side effects in compare_type for bytestrings

Cute Animal Picture

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 57.83133% with 35 lines in your changes missing coverage. Please review.

Project coverage is 52.15%. Comparing base (48a5da4) to head (b91e442).

Files with missing lines	Patch %	Lines
vyper/builtins/functions.py	63.88%	12 Missing and 1 partial ⚠️
vyper/codegen/external_call.py	33.33%	6 Missing ⚠️
vyper/semantics/analysis/local.py	28.57%	3 Missing and 2 partials ⚠️
vyper/semantics/analysis/utils.py	40.00%	3 Missing ⚠️
vyper/semantics/types/base.py	40.00%	2 Missing and 1 partial ⚠️
vyper/semantics/types/bytestrings.py	75.00%	1 Missing and 2 partials ⚠️
vyper/builtins/_signatures.py	60.00%	1 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (48a5da4) and HEAD (b91e442). Click for more details.

HEAD has 122 uploads less than BASE

Flag	BASE (48a5da4)	HEAD (b91e442)
	140	18

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3379       +/-   ##
===========================================
- Coverage   91.35%   52.15%   -39.21%     
===========================================
  Files         109      109               
  Lines       15636    15618       -18     
  Branches     3436     3433        -3     
===========================================
- Hits        14285     8145     -6140     
- Misses        921     6860     +5939     
- Partials      430      613      +183     

Flag	Coverage Δ
	52.11% <57.83%> (-39.14%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tserg]

Last failing test may be resolved by #3375

[charles-cooper]

i'm not sure it's such an easy fix. the original intent of this implementation (as far as i can tell) is to deal with progressive widening of abi types. for instance,

import jsonabi as SomeInterface  # say one function t() which returns `bytes`

...

@external
def foo():
  x: Bytes[9] = SomeInterface.t()  # t.return_type.min_length is 9
  y: Bytes[10] = SomeInterface.t()  # t.return_type.min_length is now 10

[tserg]

i'm not sure it's such an easy fix. the original intent of this implementation (as far as i can tell) is to deal with progressive widening of abi types. for instance,

import jsonabi as SomeInterface  # say one function t() which returns `bytes`

...

@external
def foo():
  x: Bytes[9] = SomeInterface.t()  # t.return_type.min_length is 9
  y: Bytes[10] = SomeInterface.t()  # t.return_type.min_length is now 10

I have added this to the test cases.

From what I understand, the changes made to _BytestringT.compare_type will allow the proper type to be derived from the type derivation helpers during annotation by relaxing the typechecking, rather than setting the length directly during typechecking before annotation.

The only other corner case is with JSON ABI imports, and other than the need to override the initial 0-length BytesT or StringT when the interface function is called, I think the compiler already handles progressive widening as is.

[charles-cooper]

i see. in that case we may be able to simplify the whole min_length thing in the type.

[tserg]

I think slice can be refactored since length_literal cannot be None if it must be a constant.

vyper/vyper/builtins/functions.py

Lines 311 to 329 in 9cc56b6

	length_literal = length_expr.value if isinstance(length_expr, vy_ast.Int) else None
	if not is_adhoc_slice:
	if length_literal is not None:
	if length_literal < 1:
	raise ArgumentException("Length cannot be less than 1", length_expr)
	if length_literal > arg_type.length:
	raise ArgumentException(f"slice out of bounds for {arg_type}", length_expr)
	if start_literal is not None:
	if start_literal > arg_type.length:
	raise ArgumentException(f"slice out of bounds for {arg_type}", start_expr)
	if length_literal is not None and start_literal + length_literal > arg_type.length:
	raise ArgumentException(f"slice out of bounds for {arg_type}", node)
	# we know the length statically
	if length_literal is not None:
	return_type.set_length(length_literal)

Should I do it here or in a follow-up PR?

[DanielSchiavini]

Is this a duplicate of charles-cooper#17 and/or charles-cooper#18?

[tserg]

Is this a duplicate of charles-cooper#17 and/or charles-cooper#18?

Yes, it does look like they are addressing the same issue.