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

poly check treats :as-alias namespace as loaded #247

Closed
seancorfield opened this issue Sep 3, 2022 · 10 comments
Closed

poly check treats :as-alias namespace as loaded #247

seancorfield opened this issue Sep 3, 2022 · 10 comments

Comments

@seancorfield
Copy link
Contributor

Describe the bug
A namespace referenced purely for an alias, via :as-alias, does not cause a circular reference, but poly check incorrectly treats it as fully-loading the namespace (it should not).

Will provide more details shortly, along with a PR since this is a critical blocker for us at work.

@tengstrand
Copy link
Collaborator

Okay, thanks for the info.
One idea is that you create a small workspace in examples, that reproduces the problem, which I can use to check my solution.

@seancorfield
Copy link
Contributor Author

I haven't had time to do this yet but might have time this weekend. I have a couple of other, very minor fixes to propose as well so I may send PRs for those soon too.

@seancorfield
Copy link
Contributor Author

Here's the change we made in our fork to simply "ignore" requires that use :as-alias (at least in terms of loaded libs).

This isn't ideal (per discussions on Slack) because it prevents poly from checking that only interfaces are used in this case. A complete solution is likely to involve quite a bit of rework in Polylith because it currently assumes that the list of required nses can be used for two purposes: checking the "rules" of component usage (good, still wanted in this case), and figuring out the dependency graph (not true in this case: :require [top-ns.component.interface :as-alias thing] does not contribute to the dependency graph -- it only introduces an alias, as if ns-alias were called).

@tengstrand
Copy link
Collaborator

I may have a simple solution to this. Will look into this today @seancorfield.

@tengstrand
Copy link
Collaborator

@tengstrand
Copy link
Collaborator

I have hopefully fixed the problem, and you can have a look at the issue-247 branch, @seancorfield.
If the code refers to another component interface using :as-alias then the :interface-deps will not be populated with that dependency, which solves the problem with circular dependencies. However, if the code refers to an implementing namespace using :as-alias then it will be included, and we will get both error 101 (Dependency on implementing namespace) and error 104 (circular dependency). The second error is in this case not correct, but you have to fix the error 101 first anyway, and it simplified the implementation a lot.

@seancorfield
Copy link
Contributor Author

That sounds like a pretty slick solution to the problem. I'll try to find time over the weekend to put together a branch based on issue-247 but with our classloader and GC patches applied and then try it on Monday at work.

@seancorfield
Copy link
Contributor Author

OK, I merged your issue-247 branch into a new branch in my fork (as-alias-247) and then cherry-picked my classloader and GC commits onto that, and I've set a reminder for Monday for me to test that as work. I should have feedback for you pretty quickly at that point.

tengstrand added a commit that referenced this issue Nov 1, 2022
Ignore reference to a component interface if required as :as-alias, see issue #247.
@seancorfield
Copy link
Contributor Author

This fix seems to be working well. Thank you!

@seancorfield
Copy link
Contributor Author

The PR for this has been merged so this issue and #211 are both completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants