[trlc] add support for nested packages - #206
Conversation
0248a13 to
2c892e5
Compare
- extent functionality - add documentation - add tests
2c892e5 to
2271930
Compare
| self.items = [] | ||
| self.package = None | ||
| self.imports = None | ||
| self.wildcard_roots = set() |
There was a problem hiding this comment.
imports starts as None and has a safety check:
assert self.imports is not None # crash if someone forgets to call resolve_imports
However, wildcard_roots starts as an empty set() rather than None. This means that if a method uses wildcard_roots before resolve_imports() has been called, it will silently behave as though there are no wildcard roots, potentially returning incorrect results instead of failing clearly.
I’m not 100% sure whether this is intentional, but it seems inconsistent with the safety check for imports. Should wildcard_roots also be initialized to None and checked before use?
| # covers the current package is permitted: the current package | ||
| # stays implicitly visible (Wildcard_Self_Cover). | ||
| if name == self.package.name and not is_wildcard: | ||
| mh.error(location, |
There was a problem hiding this comment.
I guess we need to add an explanation for this error.
| try: | ||
| a_import = stab.lookup(mh, t_import, Package) | ||
| self.imports.add(a_import) | ||
| a_import.set_ast_link(t_import) |
There was a problem hiding this comment.
This is important code. In the older code, it goes to the definition. Should we need to document to the changelog?
| else: | ||
| return name, None, None | ||
|
|
||
| def parse_dotted_name(self): |
| for root in parser.cu.wildcard_roots: | ||
| for other in self.stab.values(ast.Package): | ||
| if other.name == pkg_name: | ||
| continue |
There was a problem hiding this comment.
this code skips the current package but what about child package
suggestion: if other.name == pkg_name or other.name.startswith(pkg_name + "."):
continue
| continue | ||
| if other.name == root.name or \ | ||
| other.name.startswith(root.name + "."): | ||
| graph[(pkg_name , kind)].add((other.name , kind)) |
| # lobster-trace: LRM.Wildcard_Import | ||
| # lobster-trace: LRM.Wildcard_Self_Cover | ||
| if parser.cu.wildcard_roots: | ||
| for root in parser.cu.wildcard_roots: |
There was a problem hiding this comment.
It checks all packages again for every wildcard and every file. In a large project, this can be slow. It would be more efficient to build the package list once before the loop.
like below:
all_packages = list(self.stab.values(ast.Package))
for root in parser.cu.wildcard_roots:
for other in all_packages:
Please check its correct way or not
| return raw_loc | ||
| return None | ||
|
|
||
| def _import_in_markup(self, file, item, include_descendants = False): |
There was a problem hiding this comment.
no spaces around = in default parameter values
| continue | ||
| if self._import_in_markup(file, item): | ||
| continue | ||
| imp_location = self._find_import_location(cu, item.name, False) |
There was a problem hiding this comment.
no spaces around = in default parameter values
| def error_messages(self): | ||
| return [m for k, m in self.messages if k in (Kind.SYS_ERROR, Kind.USER_ERROR)] | ||
|
|
||
| def clear(self): |
There was a problem hiding this comment.
This method is not sued anywhere in the test file
|
The existing nested-packages-wildcard-self-cover only tests a sibling foo.baz It doesn't test when the self-covering wildcard package also has its own child. |
|
The LRM explicitly says: "importing foo.bar does NOT give you access to foo" This is a fundamental rule that has no dedicated test. |
|
Every new mh.error(), mh.warning(), and mh.check() call added in this PR is missing the explanation= argument. The project rule (copilot-instructions.md) says: For user-facing diagnostics, do not leave explanation as None unless there is truly no actionable guidance possible. Prefer explanations that help users fix the issue. |
2 participants
#42