Conversation
mbenke
left a comment
There was a problem hiding this comment.
Alas, this PR breaks the contract tests (ERC20 et consortes):
$ ./run_contests.sh
Processing: test/examples/dispatch/basic.json
Compiling to Hull...
Configuration is affected by the following files:
- cabal.project.local
Emitting hull for contract C
Writing to output1.hull
Generating Yul...
Configuration is affected by the following files:
- cabal.project.local
yule: /home/ben/work/solcore/build/output1.hull:345:34:
|
345 | return dispatch_$impl$std.$impl$abi_decode$calldataLbytesJ_pairLuint256_pairLuint256_uint256JJ_CalldataWordReader_pairLuint256_pairLuint256_uint256JJ(decodable, pty, prdr)
| ^
unexpected '.'
expecting "assembly", "false", "fst", "function", "if", "in", "inl", "inr", "let", "match", "return", "revert", "snd", "true", '$', '(', '_', '{', '}', alphanumeric character, integer, or letter
CallStack (from HasCallStack):
error, called at src/Common/LightYear.hs:26:15 in sol-core-0.0.0.0-inplace:Common.LightYear
runMyParser', called at src/Common/LightYear.hs:21:22 in sol-core-0.0.0.0-inplace:Common.LightYear
runMyParser, called at src/Language/Hull/Parser.hs:30:24 in sol-core-0.0.0.0-inplace:Language.Hull.Parser
Error: yule generation failed
Processing: test/examples/dispatch/neg.json
Compiling to Hull...
Configuration is affected by the following files:
- cabal.project.local
Module validation failed for /home/ben/work/solcore/test/examples/dispatch/neg.solc:
Undefined type constructor:
uint256
- in:function negPair () -> uint256 {
return uint256(fromB(pairfst(Neg.neg(Pair(B.F, B.T))))) ;
}
- in:function negPair () -> uint256 {
return uint256(fromB(pairfst(Neg.neg(Pair(B.F, B.T))))) ;
}
- in:contract NegPair {
constructor () {
}
function negPair () -> uint256 {
return uint256(fromB(pairfst(Neg.neg(Pair(B.F, B.T))))) ;
}
}
- in:contract NegPair {
constructor () {
}
function negPair () -> uint256 {
return uint256(fromB(pairfst(Neg.neg(Pair(B.F, B.T))))) ;
}
}
Error: sol-core compilation failed
Processing: test/examples/dispatch/miniERC20.json
Compiling to Hull...
Configuration is affected by the following files:
- cabal.project.local
Emitting hull for contract MiniERC20
Writing to output1.hull
Generating Yul...
Configuration is affected by the following files:
- cabal.project.local
yule: /home/ben/work/solcore/build/output1.hull:1294:34:
|
1294 | return dispatch_$impl$std.$impl$abi_decode$calldataLbytesJ_address_CalldataWordReader_address(decodable, pty, prdr)
| ^
unexpected '.'
expecting "assembly", "false", "fst", "function", "if", "in", "inl", "inr", "let", "match", "return", "revert", "snd", "true", '$', '(', '_', '{', '}', alphanumeric character, integer, or letter
CallStack (from HasCallStack):
error, called at src/Common/LightYear.hs:26:15 in sol-core-0.0.0.0-inplace:Common.LightYear
runMyParser', called at src/Common/LightYear.hs:21:22 in sol-core-0.0.0.0-inplace:Common.LightYear
runMyParser, called at src/Language/Hull/Parser.hs:30:24 in sol-core-0.0.0.0-inplace:Language.Hull.Parser
Error: yule generation failed
Processing: test/examples/dispatch/Revert.json
Compiling to Hull...
Configuration is affected by the following files:
- cabal.project.local
Emitting hull for contract Foo
Writing to output1.hull
Generating Yul...
Configuration is affected by the following files:
- cabal.project.local
yule: /home/ben/work/solcore/build/output1.hull:200:34:
|
200 | return dispatch_$impl$std.$impl$abi_encode$uint256(val)
| ^
unexpected '.'
expecting "assembly", "false", "fst", "function", "if", "in", "inl", "inr", "let", "match", "return", "revert", "snd", "true", '$', '(', '_', '{', '}', alphanumeric character, integer, or letter
CallStack (from HasCallStack):
error, called at src/Common/LightYear.hs:26:15 in sol-core-0.0.0.0-inplace:Common.LightYear
runMyParser', called at src/Common/LightYear.hs:21:22 in sol-core-0.0.0.0-inplace:Common.LightYear
runMyParser, called at src/Language/Hull/Parser.hs:30:24 in sol-core-0.0.0.0-inplace:Language.Hull.Parser
Error: yule generation failed
|
Oops, I forgot to run contract tests. Fixed. |
rodrigogribeiro
left a comment
There was a problem hiding this comment.
Great work! LGTM.
|
Pragma handling for imports sometimes leads to some weird errors, e.g. crashes with |
There was a problem hiding this comment.
👏 Impressive work!
I have left some suggestions and requests for clarification in the comments. My main concerns are:
- Problem with shorthand, e.g.
function join(mmx:Option(Option(word))) -> Option(word){
match (mmx:Option(Option((word)))) {
| .Some(.Some(x)) => return Option.Some(x);
| _ => return Option.None;
}
}
does not compile (cannot resolve nested .Some pattern)
2. pragma handling leading to weird bugs (see #321 (comment))
Some suggestions for future refactoring (probably not worth it right now):
- Try to eliminate boilerplate in
renameExpFunctionCalls,renameExpTypeRefs,stmtFunctionRefs,expFunctionRefs, etc. - Loader.hs is doing too much. At 1336 lines, it handles:
graph loading, import/export validation, name flattening for
validation, name flattening for compilation, function renaming,
type renaming, expression renaming, pattern renaming, function
dependency closure analysis, shadowing, and more. The AST
rewriting functions (lines ~426–1146) are a distinct concern
from the module graph logic (lines ~27–177). Splitting into at
least Loader.hs (graph + validation) and ModuleFlattener.hs
(AST rewriting) would make it more maintainable.
| import std.{*}; | ||
| import std.{Proxy, Typedef, Sub, uint256, address, bytes4, memory, calldata, string, bytes, ABIAttribs, ABIDecoder, ABIDecode, ABIEncode, CalldataWordReader, keccakLit, abi_decode, abi_encode, get_free_memory}; |
There was a problem hiding this comment.
Why are both imports needed?
| | ExpName (Maybe Exp) Name [Exp] -- function call or constructor | ||
| | ExpVar (Maybe Exp) Name -- variables or field access | ||
| | ExpDotName Name [Exp] -- contextual constructor shorthand, e.g. .Some(1) | ||
| | ExpDotVar Name -- contextual constructor shorthand, e.g. .None |
There was a problem hiding this comment.
Why is ExpDotVar needed (and with a confusing name, from the comment and the parser it seems it does not represent a variable. IOW, why is .None represented as ExpDotVar "None" rather than `ExpDotName "None" []"?
| %right 'else' | ||
|
|
||
| %expect 0 | ||
| %expect 1 |
There was a problem hiding this comment.
This deserves a comment explaining which conflict exists and why the default resolution is correct.
| | .Some(v) => return v; | ||
| | .None => return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
OTOH, this does not work, despite inserting as much type annotations as possible:
function join(mmx:Option(Option(word))) -> Option(word){
match (mmx:Option(Option((word)))) {
| .Some(.Some(x)) => return Option.Some(x);
| _ => return Option.None;
}
}
the problem is apparently in nested constructors
|
|
||
| flattenModuleCompUnit :: ModuleGraph -> FilePath -> Either String CompUnit | ||
| flattenModuleCompUnit graph modulePath = | ||
| flattenModuleValidationCompUnit graph modulePath |
There was a problem hiding this comment.
Why are two functions needed? What is the difference?
|
|
||
| - Canonical constructor names are type-qualified (`Bool.True`, `Option.Some`). | ||
| - Module-qualified constructor access is supported (`mod.Bool.True`, `alias.Bool.True`). | ||
| - Unqualified constructors are generally rejected when only qualified constructors exist. |
This PR is a PoC for the new module/namespace system and focuses on language behavior and import/export semantics.
The current implementation keeps a transitional flattening bridge so existing passes continue to work. If this direction is accepted, I'll start refactoring to remove that bridge by introducing module-aware lookup in name resolution/type checking and then cleaning up related paths.
TODOs:
Module and Namespace System
1. Entry File and Import Roots
-f/--file.takeDirectory(entryFile)--include(colon-separated, default:std)2. Module Identity and File Mapping
import foo;resolves tofoo.solc.import foo.bar;resolves tofoo/bar.solc.3. Import Syntax and Semantics
Supported forms:
Behavior:
import M;andimport M as A;add qualifier-based access only.import M.{X, Y};imports selected exported names into unqualified scope.import M.{*};imports all exported names into unqualified scope.Validation rules:
import A as M; import B as M;).Data constructor selection detail:
{True}) brings its parent data type declaration too, but only with selected constructors.4. Export Syntax and Visibility
Supported forms:
Current enforcement:
export {*};exports all importable top-level declarations (except pragma/export declarations).5. Namespaces and Name Resolution
Duplicate checking is enforced separately for:
Unqualified lookup order:
import M.{...}/import M.{*}Module qualification:
import foo.bar;), prefix qualifiers are also registered for qualified access paths.6. Constructor Model
Current constructor model is type-qualified constructors, with dot shorthand support.
Bool.True,Option.Some).mod.Bool.True,alias.Bool.True).Dot shorthand:
7. Pragma Scope