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

Dependency by inheritance is permitted #35

Open
gimmi opened this issue May 28, 2018 · 7 comments
Open

Dependency by inheritance is permitted #35

gimmi opened this issue May 28, 2018 · 7 comments
Milestone

Comments

@gimmi
Copy link

gimmi commented May 28, 2018

Let's say i want to separate data access logic from UI, and to enforce it i will create MyApp.UI and MyApp.Data namespaces with the following rules:

<Allowed From="MyApp.UI" To="MyApp.Data" />
<Allowed From="MyApp.Data" To="System.Data" />

What I want to enforce is that using System.Data from MyApp.UI is not permitted, and this seems to work properly. However if I create a class like this:

namespace MyApp.Data
{
    public class MyDataColumn : System.Data.DataColumn
    {
    }
}

I am able to use it in MyApp.UI, and this will generate a dependency on System.Data.

Is this intentional?

@realvizu
Copy link
Owner

I'm not sure if I understand the situation. There's no special treatment for inheritance chains in the tool.
The scenario that you have described should not work unless you have also allowed the following rules:

  • SpikeNsDepCop.Data -> System.Data
    • (because MyDataColumn requires it to be able to inherit from DataColumn)
  • MyApp.UI -> SpikeNsDepCop.Data
    • (because you said you can use MyDataColumn in MyApp.UI)

If you haven't allowed these dependencies then there's something wrong. Can you send me your config.nsdepcop file so I can take a look?

@gimmi
Copy link
Author

gimmi commented May 30, 2018

Hi @realvizu

yes, in the example I did some namespace wrong. I' ve edited and fixed it and what I meant is exactly your guess.

What I'm trying to do is to make MyApp.UI independent from System.Data, but currently seems that I am not able to prevent this if I create a class in MyApp.Data namespace that inherit from a System.Data class.

Is there any plan to support this scenario?

@realvizu
Copy link
Owner

So you're suggesting that the dependency analyzer should track down transitive (indirect) dependencies by checking base types recursively, right?

Well, I haven't considered such feature so far. There are a number of questions that come to my mind. Can you help elaborating on them a bit?

  • Is it just class inheritance or are there other kinds of dependencies that should be detected transitively (interface implementation, interface inheritance, ...) ?

  • How valueable could such feature be? I mean, what are the problem scenarios that this could prevent? After all, when you have an indirect dependency, you also have a layer of indirection that helps to mitigate the coupling problems that can arise from the dependency.

  • How severe could be the performance implication of transitive (recursive) dependency checking? (this is not a real question, just a concern as it cannot be answered upfront just after implementing and measuring)

@gimmi
Copy link
Author

gimmi commented May 31, 2018

I wouldn't call this kind of dependency a transitive dependency.

I mean, if I have a class Class1 that uses Class2 that in turn uses Class3, then Class1 has a transitive dependency on Class3. This is NOT the scenatrio I'm trying to control with NsDepCop.

If instead I have a class Class1 that uses Class2 that inherit from Class3, then in order to compile Class1 you also need to reference the assembly that contains Class3. I think being able to control this kind of dependency with NsDepCop would be useful.

@realvizu
Copy link
Owner

realvizu commented Jun 3, 2018

Well, just to make sure I'm not mistaken, I've just tried the scenario that you have described:
Class1 --uses-> Class2 --inherits--> Class3
And to compile it I did not have to reference Class3's assembly from Class1's assembly (only Class2's assembly). Of course there is an indirect runtime dependency from Class1 to Class3 but the compile-time dependency is only between Class1 and Class2 (and of course between Class2 and Class3, but that's beside the point).
So I still don't see why would be the inheritance relationship special compared to any other code dependencies when it comes to transitive dependency checking. In my point of view you either care about indirect dependencies or you don't, but out of all transitive relationships checking only the inheritance dependencies seems a bit arbitrary for me.

Don't get me wrong, I'm not trying to dismiss your feature idea but I'd like to understand well its value and whether it conceptually fits NsDepCop before putting effort into the implementation.

gimmi added a commit to gimmi/NsDepCop that referenced this issue Jun 3, 2018
@gimmi
Copy link
Author

gimmi commented Jun 3, 2018

Hi @realvizu

And to compile it I did not have to reference Class3's assembly from Class1's assembly

Maybe you just created the class, and not invoked any of the inherited methods?

Anyway, take a look at my fork (74ec4c7) where I've created a sample for the feature request. Basically you can work around NsDepCop by creating a "proxy" class in an allowed namespace that just inherith from the forbidden class you want to use.

@realvizu
Copy link
Owner

realvizu commented Jun 5, 2018

Ok, I see your point.
I'll assign it to the next release (v1.9).
Thanks, for being persistent. :)

@realvizu realvizu added this to the v1.9 milestone Jun 5, 2018
@realvizu realvizu modified the milestones: v1.9, v2.x Feb 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants