Bug fix: don't clobber repl context variables when setting up repl context

[eggplantzzz]

As mentioned in #3329, if a user has a contract name that conflicts with one of Node's native objects (such as Buffer) then it will overwrite that object. This could cause things to break. In order to fix this, this PR provides an implementation where Truffle will not overwrite objects found in the context the first time it loads the repl and sets up the environment. The provision method is where these things are set up, and on subsequent calls to provision it will overwrite things it finds in the repl context as it will need to update contracts that have changed etc.

[cliffoo]

Nicely done @eggplantzzz ! I verified that:

• If contract name doesn't conflict, hydrate to repl.
• If conflict, contract available via artifacts.require

Which I think is very sensible! We definitely need to document this though.

Two more things:

• We prob want tests for this? Though I think that can come later and this PR shouldn't be held back.
• Release: I consider this breaking because there may be users relying on the current behavior to overwrite node's built-in objects.

[eggplantzzz]

So I found an issue with the current implementation here that needs to be fixed. Currently it won't clobber names in the context the first call to provision but will subsequent times. This is partially necessary because we have to provide fresh copies of all artifact objects after each command as they could have changed/been updated.

What we really need to do is make a note of all name conflicts on the first provision. On subsequent calls to provision we should reference this list and not overwrite any variables that conflicted on the first call (as those will be the objects that we do not want to overwrite).

an update is necessary since an issue has been found with the current implementation

[dongmingh]

I tested it with Buffer.sol with Buffer contract and compilation succeeds and const Buffer = artifacts.require("Buffer") works. Then I run truffle again with json in the build directory, I see the following warning message:

truffle(develop)> 
 > Warning: One or more of your contract(s) has a name conflict with something in the current repl context and was not loaded by default. 
 > You can use 'artifacts.require("<artifactName>")' to obtain a reference to your contract(s). 
 > Truffle recommends that you rename your contract to avoid problems. 
 > The following name conflicts exist: Buffer, ConvertLib, MetaCoin.

And the truffle prompt does not return. I need to hit return key to see the truffle prompt again:

undefined
truffle(develop)>

[eggplantzzz]

Hmmm @dongmingh you shouldn't be getting warnings for things like MetaCoin and ConvertLib. Can you tell me more exactly what you did?

Edit: I figured out what you did :)

[dongmingh]

@eggplantzzz let me know if need me to show you how I saw this.

[eggplantzzz]

Thanks @dongmingh, I found a bug with how contracts are detected and fixed it. You should no longer be getting conflicts for things like MetaCoin. However, the prompt still does not appear when the warning is present.

[eggplantzzz]

Ok I also caused the prompt to display after provisioning which will cause the warning to display before the prompt. So the other bug you found @dongmingh should be fixed now.

[dongmingh]

I retest it and no longer see conflict contracts like MetaCoin. Also the truffle prompt displays as expected. Thanks.

[cds-amal]

Thanks, @eggplantzzz, this looks better! I left a few nits and requesting changes for testing.

[cds-amal]

Had a scenario question.

[cds-amal]

I consider this breaking because there may be users relying on the current behavior to overwrite node's built-in objects.

@clifoo, what if we consider clobbering a bug, so this can be a patch instead of breaking? What do you think?

[cliffoo]

I believe that's the conclusion @eggplantzzz mentioned earlier, though I'm not fully convinced. This reminds me of the recent integer/string conversion change pushed to python as a patch release (short video for context). Yes it was a bug, they fixed it, yet it broke things anyway.

Similarly, this is bug fix and it's breaking. (My Math and Buffer contracts were just fine, what happened!)

[cds-amal]

Thanks @cliffoo. I'm on team breaking on this one.

Edit: This actually fixes a #3329, so it should be a patch.

[eggplantzzz]

If we consider this a breaking change we can't get it in until v6. Originally through discussion with @gnidan we decided this was a bug fix. We can revisit and discuss this again.

My specific concerns were addressed

[cds-amal]

I verified this works, which is really cool. I left some minor nits, and requesting a rework of the display conflict alert logic.

[cds-amal]

I kept coming back to this because something was bugging me. Unless I'm missing something, this logic can be simplified if we track two pieces of state:

1. knownCollisionsSoFar
2. newlyDiscoveredCollisions

• fn recordNameConflicts would add update the newlyDiscoveredCollisions container IFF new abstraction isn't in knownCollisionsSoFar
• fn resetContractsInConsoleContext would:
  1. load the abstraction in context if the name isn't in both newlyDiscoveredCollisions and knownCollisionsSoFar
  2. alert user if newlyDiscoveredCollisions has items
  3. move newlyDiscoveredCollisions to knownCollisionsSoFar
  4. reset newlyDiscoveredCollisions

Of course, please correct me if I missed a key piece of the logic.

[cds-amal]

This is much easier to follow! I left a stylistic comment, feel free to ignore; and a change request to use Set.keys() instead of Set.entries() in the for of loop.

[cds-amal]

LGTM. Thanks, @eggplantzzz!