Added module system support for diamond and repeated imports, and to prevent recursive imports. #443
Conversation
supercooldave
changed the title
| importAndCheckModules :: (Environment -> Program -> IO (Environment, Program)) -> | ||
| [FilePath] -> Program -> IO ModuleMap | ||
| importAndCheckModules checker importDirs p = do | ||
| (modules, env) <- importAndCheckProgram [] Map.empty emptyEnv [] p -- [] is QName of top-level module. |
There was a problem hiding this comment.
env variable is never used
|
rebase this branch on top of |
|
@kikofernandez I've addressed your changes and rebased. |
| importAndCheckProgram seen importTable env target impl | ||
|
|
||
| importOne :: [FilePath] -> ImportDecl -> IO Program | ||
| importOne importDirs (Import _ target) = do |
There was a problem hiding this comment.
I am looking at this function but, somehow, it's not clear to me. My understanding:
importOnetakes always the same input forimportDirs(which comes from theimportDirsfromimportAndCheckModules checker importDirs p)- Append to the dirs the target
- Filter out the ones that do not exist
- If there is more than one, e.g.
libA/B/MapIntandlibC/D/MapInt, the compiler throws a warning. IflibAandlibCare the includes (-I libA:libC), the compiler can still distinguish which module we want to access to, eitherB/MapIntorC/MapInt, and the warning could be avoided. Does this make sense?
However, I can understand the warning if we include -I libA:libB and we have libA/B/MapInt.enc and libB/B/MapInt.enc and the user imports the module import B.MapInt. There is no way of telling which of the two we are accessing. In this case, an error is better (I think).
Does this make sense or am I mistaken somewhere along the road?
There was a problem hiding this comment.
You understand correctly.
I'll see what the impact of converting the warning into an error is.
There was a problem hiding this comment.
Converting it to an error means I can convert a succeeding test into a failing test – which is how it should be.
Good catch.
|
test suite seems to complain that there is a missing |
|
|
||
|
|
||
| compressModules :: ModuleMap -> Program | ||
| compressModules = foldl1 joinTwo |
There was a problem hiding this comment.
does this work (in the sense that you are guaranteed to have at least 2 Programs) because you are always adding the String module? (in the future, you will add stds)
There was a problem hiding this comment.
Are you referring to fold1? It requires only one element in the list.
There was a problem hiding this comment.
you are right, sorry. I thought it requires two arguments by default.
|
Regarding |
| (Left err, warnings) -> (Left err, warnings) | ||
|
|
||
| -- addEnvironment :: (Either TCError Program, [TCWarning]) -> (Either TCError (Environment, Program), [TCWarning]) |
There was a problem hiding this comment.
remove unless you are planning on using this in the next iteration of the module system
|
I have finished with the PR and will check only the incremental changes. After that, this PR has my blessing and will be merged (probably Monday, I am taking the weekend off) |
|
@kikofernandez I addressed your comment about commented out debugging code in a better way (in last push). |
|
Merging in 1h |
Added module system to support diamond and repeated imports, and prevent recursive imports.
Involved a significant reworking of the way modules were included in the code, and hence touched quite a few places in the code.
Fixes #329 and #233.
This PR lays the foundation for proper module support, including namespaces and all the other things you would expect.
Several new test cases provided. Failing tests are not yet integrated into the test build, awaiting @kaeluka's new test scripts.
No new documentation provided, as these changes make the modules behave closer to what is expected.