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: 33 lines in your changes are missing coverage. Please review.

Comparison is base (b0ea5b6) 83.98% compared to head (34ab4bc) 58.31%.

Files	Patch %	Lines
vyper/builtins/functions.py	78.43%	10 Missing and 1 partial :warning:
vyper/semantics/analysis/local.py	60.00%	2 Missing and 4 partials :warning:
vyper/codegen/external_call.py	28.57%	5 Missing :warning:
vyper/semantics/types/base.py	50.00%	1 Missing and 2 partials :warning:
vyper/semantics/types/bytestrings.py	75.00%	1 Missing and 2 partials :warning:
vyper/builtins/_signatures.py	75.00%	1 Missing and 1 partial :warning:
vyper/semantics/types/function.py	66.66%	1 Missing and 1 partial :warning:
vyper/semantics/analysis/utils.py	87.50%	1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3379       +/-   ##
===========================================
- Coverage   83.98%   58.31%   -25.68%     
===========================================
  Files          92       92               
  Lines       13046    13026       -20     
  Branches     2928     2925        -3     
===========================================
- Hits        10957     7596     -3361     
- Misses       1657     4801     +3144     
- Partials      432      629      +197     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[tserg]

Last failing test may be resolved by https://github.com/vyperlang/vyper/pull/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. https://github.com/vyperlang/vyper/blob/9cc56b618a54e6144829d6c056feb43e00c9c5fe/vyper/builtins/functions.py#L311-L329

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

[charles-cooper]

for some reason i thought this was merged already. @tserg can you fix the merge conflicts and we can review this again?

[DanielSchiavini]

Is this a duplicate of https://github.com/charles-cooper/vyper/pull/17 and/or https://github.com/charles-cooper/vyper/pull/18?

[tserg]

Is this a duplicate of https://github.com/charles-cooper/vyper/pull/17 and/or https://github.com/charles-cooper/vyper/pull/18?

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