Provide type support to linker.ts

[stephen-slm]

Why

This change provides some basic type support for the internal and exported methods within linker.ts, includes support documentation where missing for the consumer. The target result of this PR is to increase readability, maintainability and start the introduction of types.

This is the continuous development happening on top of #566 to improve user consumption, and development experience.

Depends on #603 and #594

Note

This is currently rebased on #603 so just validate the latest commit.

[coveralls]

Coverage remained the same at 83.721% when pulling 6409860 on stephensli:types/linker into f54dabf on ethereum:master.

[cameel]

Let's call it fullyQualifiedLibraryName to avoid any confusion. I.e. it's supposed to include the file name and the library name joined with :.

Also, by two-level configuration you mean {"lib.sol": {"L": "0x..."}}? Then the names don't really match. I.e. then the first level is not really a library name but the file name (or source unit name to be precise).

[stephen-slm]

The type is to support both methods which are currently passed in.

{"lib.sol": {"L": "0x..."}}
and
{"lib.sol": "0x..."}

which I experienced and gained from writing the changes the tests. If i'm wrong we can correct the names.

[cameel]

The second should be: {"lib.sol:L": "0x..."}.

Well, technically {"lib.sol": "0x..."} will still work but only if you have a placeholder with lib.sol in it. Which you won't get from actual compilation but tests could be set up with a situation like this. lib.sol here would be interpreted as library name, i.e. as if you wrote library lib.sol {} in your code, which is forbidden due to the dot.

[stephen-slm]

thoughts?

// kind 1: {"lib.sol:L": "0x..."}
// kind 2: {"lib.sol": {"L": "0x..."}}

// we can specify the requirement for `L` if its always going to be `L` or change it to `[x: string]`
type: { [x: string]: string } | { [x: string]: { "L": string }  } 

[cameel]

Unfortunately the current code supports mixing the two. E.g. this is valid input:

{
    "lib.sol:L1": "0x...",
    "lib.sol:L2": "0x...",
    "lib.sol": {"L3": "0x..."}
}

On the other hand, we don't even support the first kind in Standard JSON and I doubt any compiler version supported both so we might be fine changing it. It's technically a breaking change for solc-js though. @axic what do you think?

So yeah, what you did in the PR is correct, just names are a bit misleading. We could do this but it's a bit ugly:

export interface LibraryAddresses {
  [qualifiedNameOrSourceUnit: string]: string | { [unqualifiedLibraryName: string]: string };

On the other hand the ugliness at least does let you know that there's something wonky going on here and you should watch your step :)

The only other idea that comes to mind is to give the first level some generic name not to suggest it's necessarily one or the other:

export interface LibraryAddresses {
  [firstLevelID: string]: string | { [unqualifiedLibraryName: string]: string };

[cameel]

Can you explain how it supports both?

type: { [x: string]: string } | { [x: string]: { [x: string]: string }  } 

Doesn't the | here mean that it must have one signature or the other and not that you can mix and match fields allowed in both?

In any case, unless it supports both, that suggestion does not match what the library supports.

The problem still stands, names and the docstring here are ambiguous and need to be adjusted.

[stephen-slm]

Since the libraries: LibraryAddresses is an object, it's stating that the content of each item within that object can be one or the other. A single item cannot be both so this is fine. In any case, unless it supports both, that suggestion does not match what the library supports. - you will have to be more clean on this since I have made all the required changes

[cameel]

Since the libraries: LibraryAddresses is an object, it's stating that the content of each item within that object can be one or the other. A single item cannot be both so this is fine.

I think we might be talking about different things here. I get the feeling you're commenting on the form you used in the PR and not on the one I cited. But no matter, that's not all that important so let's drop it. Perhaps I'm just misunderstanding this. I was just giving feedback on your suggestion from #604 (comment) because you asked for it.

The version in the PR is fine. I'd just replace the two level configuration part in the docstring with the example of two kinds you have above.

[stephen-slm]

sounds good,

i'll add this example

{
    "lib.sol:L1": "0x...",
    "lib.sol:L2": "0x...",
    "lib.sol": {"L3": "0x..."}
}

[stephen-slm]

done

[cameel]

Unfortunately the current code supports mixing the two. E.g. this is valid input:

{
    "lib.sol:L1": "0x...",
    "lib.sol:L2": "0x...",
    "lib.sol": {"L3": "0x..."}
}

On the other hand, we don't even support the first kind in Standard JSON and I doubt any compiler version supported both so we might be fine changing it. It's technically a breaking change for solc-js though. @axic what do you think?

So yeah, what you did in the PR is correct, just names are a bit misleading. We could do this but it's a bit ugly:

export interface LibraryAddresses {
  [qualifiedNameOrSourceUnit: string]: string | { [unqualifiedLibraryName: string]: string };

On the other hand the ugliness at least does let you know that there's something wonky going on here and you should watch your step :)

The only other idea that comes to mind is to give the first level some generic name not to suggest it's necessarily one or the other:

export interface LibraryAddresses {
  [firstLevelID: string]: string | { [unqualifiedLibraryName: string]: string };

[cameel]

I think that one of the docstrings still needs small adjustments but this is not a critical issue so I'm approving already.

@axic Could you take a final look at this?