ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.46k stars 5.8k forks source link

Disallow trailing slashes in imports #10980

Open cameel opened 3 years ago

cameel commented 3 years ago

From https://github.com/ethereum/solidity/pull/10976#discussion_r578594268

Abstract

Currently the compiler ignores any number of slashes at the end of the name of an imported module. For example this test case is perfectly valid:

==== Source: a/b/c.sol ====
contract C {}
==== Source: a/b/d.sol ====
import "./c.sol/";
contract D is C {}

I think that this should result in a compilation error instead. Only import "./c.sol"; should be accepted.

Motivation

I see no legitimate need for these trailing slashes. I haven't seen this used in practice and if I did, I would probably be confused by it - Solidity does not currently allow importing whole directories but it looks like such an import.

It's also a source of bugs due to libraries not supporting all edge cases correctly (e.g. #10713). Disallowing this would free us from worrying about some of these edge cases.

Specification

If the path passed to import ends with /, the compiler should issue an error saying that it's not possible to import a directory. For consistency, the compiler might also issue this error if it determines that an import not ending with a slash is pointing at a directory.

Alternatively, given that user might not understand that the compiler sees a file with a trailing slash as a directory, the message could just say that imported paths cannot end with a slash.

Backwards Compatibility

This will break code that uses this style of import. Such code is easy to fix by removing the slash but it's still a breaking change.

chriseth commented 3 years ago

We should restrict both filenames (at the level of CompilerStack / StandardCompiler / CommandlineInterface) and import paths to not end in slashes and not being empty. I don't think we need a general discussion about that.

cameel commented 3 years ago

I don't really see a problem with allowing a single slash at the end of paths, though disallowing it is also not a big deal.

If you think this is a good change that does not require discussion then I'll just move it to the implementation backlog. Do you agree it's a breaking change? Technically, allowing the slash could also be seen as a bug and I doubt many people rely on that.

Marenz commented 3 years ago

I would disallow it but also maybe add the feature to be able to import paths?

cameel commented 3 years ago

So it would let you wholesale import everything from all the .sol files in that directory?

I think it would be an anti-feature for people auditing contracts. It would make it even harder to track down where something is being imported from. If it was up to me, I would go the other way and only allow the import X from "Y.sol"; form. Or at least require prefixing everything with Y. if you only do import "Y.sol"; :)

cameel commented 3 years ago

Looks like trailing slashes are only ignored when using paths relative to the current file (i.e. starting with ./ or ../). In absolute and normal relative imports they are not. See examples in #11036.

So this looks like like an artifact of path normalization than an intentional feature.