Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: adds modules and visibility #283

Merged
merged 1 commit into from Nov 4, 2020

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented Oct 10, 2020

This (still way too large, Im sorry 😓 ) PR adds use of pub keywords and resolved visibility. It also introduces the concept of modules which are ordered in a hierarchical manner based on their location on disk. This is required to be able to specify visibility scopes. This PR also (again Im sorry for the size 😓 ) introduces the concept of definitions from all over the package in the form of PackageDefs. This can later be used to more easily implement use as well.

All of this required quite the overhaul, especially because we have modules now instead of files.

Closes #248

This PR is still in Draft because I want to:

  • add diagnostics for missing module files (foo/ exists but no mod.rs or foo.rs). We decided that that is actually not an error.
  • add more tests!
  • test visibility resolution
  • Actually check visibility in paths

@baszalmstra baszalmstra added type: feat New feature or request type: refactor Refactor existing code labels Oct 10, 2020
@baszalmstra baszalmstra self-assigned this Oct 10, 2020
@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #283 into master will increase coverage by 0.39%.
The diff coverage is 80.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   79.34%   79.73%   +0.39%     
==========================================
  Files         220      228       +8     
  Lines       13069    13500     +431     
==========================================
+ Hits        10369    10764     +395     
- Misses       2700     2736      +36     
Impacted Files Coverage Δ
crates/mun_compiler_daemon/src/lib.rs 0.00% <0.00%> (ø)
crates/mun_hir/src/item_tree/tests.rs 100.00% <ø> (ø)
crates/mun_language_server/src/diagnostics.rs 0.00% <0.00%> (ø)
crates/mun_language_server/src/main_loop.rs 23.21% <0.00%> (ø)
crates/mun_syntax/src/ast.rs 80.00% <ø> (ø)
crates/mun_syntax/src/ast/generated.rs 76.14% <ø> (+0.91%) ⬆️
...ates/mun_syntax/src/parsing/grammar/expressions.rs 94.02% <ø> (ø)
crates/mun_syntax/src/parsing/grammar/patterns.rs 90.47% <ø> (ø)
crates/mun_syntax/src/parsing/grammar/types.rs 100.00% <ø> (ø)
crates/mun_test/src/fixture.rs 92.85% <ø> (ø)
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6f6c40...9da3be5. Read the comment docs.

@baszalmstra baszalmstra changed the title Feature/pub feature: adds modules and visibility Oct 14, 2020
@baszalmstra baszalmstra marked this pull request as ready for review October 14, 2020 16:56
Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy to see this work nearing completion. Mun v0.3 is shaping up nicely!

This is only a partial review, but I wanted to make sure that part of my comments were already actionable. I'll continue the review later.

crates/mun_codegen/src/ir/file_group.rs Outdated Show resolved Hide resolved
crates/mun_compiler/src/driver.rs Outdated Show resolved Hide resolved
crates/mun_compiler_daemon/src/lib.rs Show resolved Hide resolved
crates/mun_hir/src/code_model/module.rs Show resolved Hide resolved
crates/mun_hir/src/code_model/package.rs Show resolved Hide resolved
crates/mun_hir/src/ty/tests.rs Show resolved Hide resolved
crates/mun_hir/src/visibility.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/visibility.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/visibility.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next batch of reviewed files 😅

crates/mun_hir/src/item_scope.rs Show resolved Hide resolved
crates/mun_hir/src/item_tree/lower.rs Show resolved Hide resolved
crates/mun_hir/src/module_tree.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/module_tree.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/module_tree.rs Show resolved Hide resolved
@baszalmstra
Copy link
Collaborator Author

I think I solved all issues, also rebased everything on current master.

Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last batch of comments. Hopefully we can merge this soon 🌲

crates/mun_hir/src/name_resolution/path_resolution.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/name_resolution/path_resolution.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/package_defs.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/package_defs/collector.rs Outdated Show resolved Hide resolved
crates/mun_hir/src/package_defs/collector.rs Show resolved Hide resolved
crates/mun_hir/src/package_defs/collector.rs Outdated Show resolved Hide resolved
@baszalmstra baszalmstra merged commit a45496a into mun-lang:master Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feat New feature or request type: refactor Refactor existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add language support for specifying pub(...) scope
2 participants