@chriseth Sorry, I should try to pass the isoltest first. :-)

I think the solution should be losing the restrict on the solcAssert in codegen because when the target is a reference it is possible for a convert from memory to storage. The convert from memory to storage reference will copy the content of the memory.

Codecov Report

Merging #4904 into develop will increase coverage by 59.36%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##           develop    #4904        /-   ##
============================================
  Coverage    28.51%   87.88%    59.36%     
============================================
  Files          314      314               
  Lines        31496    31510        14     
  Branches      3730     3721        -9     
============================================
  Hits          8981    27691     18710     
  Misses       21835     2557    -19278     
- Partials       680     1262       582

Finally, I found that explicit type conversion for reference type will always try to convert the type to a reference type. Solidity will modify the is_pointer and location part if it is a explicit type conversion.

However, the explicit type conversion for an array will call the implicit type conversion. But, the check for an implicit conversion to a storage reference is very weak because the function believes that this conversion will cause data copy. But, explicit type conversion will not cause any data copy.

So, this bug is fixed by distinguishing the explicit type conversion with implicit type conversion.

Ah great, that sounds much better! I'm wondering whether we should rather fix that explicit conversion converts to reference, because that does not sound right.

I actually wonder where we have a valid conversion to storage pointer anyway. Did you find an instance of that? Perhaps contract c { string x; function f() { bytes(x); } }

Hm, so perhaps an earlier version of yours did go in the right direction: conversion between string and bytes is a very special case, because the base types are different, but we can convert without actually copying anything.

Yeah, two types can convert if and only if the layout and the size are the same.

Except that we want to have some constraint on the memory.
Anyway, the convert from into[10] storage to int[] storage shouldn't be allowed without copying since the layout is totally different.

Could you add an assert or check that an explicit conversion to pointer is only allowed for a byte/string conversion?

FYI, the following code has no problem since it is a conversion to "int[] storage ref" that caused a copy.

➜  liang@matrix ~/projects/zzz cat t1.sol 
pragma solidity ^0.4.15;
contract Hello {
  int[10] x;
  int[] y;
  function f() public {
    y = x;
  }
}
➜  liang@matrix ~/projects/zzz solc t1.sol 
Warning: This is a pre-release compiler version, please do not use it in production.
➜  liang@matrix ~/projects/zzz 

I am not quite sure if I understand you well on adding the assertion. Current version only allow the basetype to be the same which is more restrict than the assertion you mentioned. The assertion seems useless.

The methodology for this fix is that the implicit conversion to reference causes copy because an expression that takes a reference as parameter requires that. However, the explicit type conversion will never cause a copy even if it is converting to a reference type. Actually, Solidity tries to avoid implicitly converting a reference to a pointer, for some reason I do not know, by changing the target pointer type to reference type automatically.

So, this fix distinguishes the explicit type conversions from implicit ones and regard them as a non-copy conversion.

I think you want to only allow the explicit conversion to a pointer for target types of bytes or string.
I added to following to lines of code. The test passed locally. Seems reasonable constraint. :-)

                if (isExplicit && convertTo.isPointer() && !convertTo.isByteArray() && !convertTo.isString())
                        return false;

@chriseth Hi, do you have any further comments on this PR. Thanks :-)

@chriseth the type check for bytes and string is moved to type checker part. I think this is the place it belongs to, and the requirement added by you also fixes the issue #4948.

The issue I see with this PR is that in some cases now implicit conversions are less restrictive than explicit conversions. In general explicit conversions should always be less restrictive than implicit ones.

Do you think it might make sense to not let an explicit conversion change from a storage reference to a storage pointer instead and thus allowing the following, instead of disallowing it?

contract C {
        uint[] a;
        uint[] b;
        function f() public view {
            uint[] storage c = a;
            uint[] storage d = b;
            d = uint[](c);
        }
}

I also think the following should be valid:

contract C {
        uint[] a;
        uint[] b;
        function f() public view {
            uint[] storage c = a;
            uint[] memory d = b;
            d = uint[](c);
        }
}

Since in both cases the same is valid as an implicit instead of an explicit conversion. @chriseth, what do you think?

In Solidity, we will regard a conversion to reference type will need to copy. Essentially, this means Solidity always regard the explicit conversion for an array will need a copy. (I thought the implicit conversion will never copy anything. I think this should be wrong. So, this PR needs to be updated accordingly.)

I guess this might be the reason why array pointer type will always be changed to a reference type because the result of an explicit conversion is an RValue (I do not have a concrete proof yet).

This might be the difference between implicit conversion and explicit conversion, e.g. implicit conversion will never happen for the x in "x=exp" but may happen for explicit conversion.

Could you add an assert or check that an explicit conversion to pointer is only allowed for a byte/string conversion?

@chriseth @ekpyron I have updated this PR to only add the restrictions. Moreover, 6 type conversion test cases are added.

The following only applies to reference types: Implicit conversion can copy. For example if you do x = y; and x and y are storage variables of different array types. Explicit conversion might copy, but since an explicitly converted value can always "stand on its own" (i.e. x = uint[](y); and (uint[](y))[2]) are both valid), an explicit conversion can only copy if the conversion converts to memory. Since explicit conversions do not change the data location, this can actually never happen from storage to memory.

We REALLY have to re-architect the copy and conversion rules... There was a proposal to require things like x = copyof(y), where copyof(y) is something like std::move(y) which only changes the type but does not generate any code.

I much prefer this version. Note that there is also an exception in CompilerUtils::convertType about byte arrays, so this matches well.

@chriseth @ekpyron
Do you think we should not change array pointer to array ref in copyForLocationIfReference?
Then, the if-statement can be changed to assert. :-)

From the name of the function, I can understand that copyForLocationIfReference will copy the location, but I am not quite sure why should we set the _isPointer.

@chriseth @ekpyron
Simply add an assertion will only trigger the assert fail earlier which will not fix the problem.

@chriseth @ekpyron
Shall I fix this by not allowing modifying the _isPointer and change the if-statement to assertion?

If we want to change the behaviour of copyForLocationIfReference to not modify m_isPointer, then we have to consider all its use cases. An example would be ReferencesResolver::endVisit(Mapping const& _typeName). If I see it correctly, here calling copyForLocationIfReference for the value type relies on it setting m_isPointer to false. Not sure whether that's the only case.

I mean only this location not set it by adding an argument with default value false.

That might make sense, yes.

Shall I try this way or wait for more discussion?

Instead of modifying copyForLocationIfReference, we could use ReferenceType::copyForLocation directly in TypeChecker.cpp#L1736, if resultType is a reference type. (I take it that's the place you're talking about?). I think it's worth a try, but you can also wait for @chriseth's response to be sure.

@ekpyron
yeah, sounds good. I will have a try first. Not a very large change.

For the external tests to pass you need to rebase the PR (they depend on #4926 being merged now).

I think the latest change is looking good, though!

rebased. Thx :-)

As I said earlier, I think the whole concept of reference, pointer and convertability has to be reworked, but I think this PR is a very good solution for now.

@ekpyron I am fine with your change. Very much easy to read now. 👍
By the way, how can you do that? I mean modifying without generating a new commit.
Did you rebase by squash?

@liangdzou In this case it was only the latest commit that I modified, so I just used git commit --amend (which modifies the last commit using the staged changes) and then git push -f (to overwrite the old commit in the remote repo). In case I want to modify a commit that's not the latest one, but further down the history, I usually create a tmp commit and then rebase and squash it into the commit I wanted to modify, but there may be more elegant ways as well :-).

@ekpyron Thanks 👍