Provide a way to avoid FileNotFound runtime errors from transitive package references that are later removed

[mungojam]

There is a fairly simple case in the new PackageReference world where a runtime error can occur where it could be prevented or flagged by maybe msbuild or the compiler. It comes about because we can happily call code that is in indirectly referenced packages, not just in the top level ones.

Detailed Scenario:

3 packages: PackA, PackB and PackC (all .net standard 1.4 in my case)

One app: App1 (.net framework or .net core)

PackA has no dependencies

PackB v1.0.0 has PackageReference to PackA (doesn't matter if it actually uses it or not)

PackC has PackageReference to PackB (v1.0.0) but directly uses method in PackA. It is unfortunately able to do this even though it doesn't explicitly list PackA as a PackageReference. Maybe the developer uses PackB so adds that and doesn't realise they were then also using PackA (easy to do with internal company packages as you tend to remember what functions exist more than what packages they live in).

App1 has PackageReference to PackC and PackB (v.1.0.0).

Everything works nicely but then...

PackB v1.0.1 is released, which removes the PackageReference to PackA.

App1 updates PackB to v1.0.1

App1 compiles but when it calls PackC which then tries to call PackA, it fails with FileNotFoundException because PackA is not known to be a dependency so does not get included in the build.

My thought is that the ideal way to fix this would be a compiler error or warning at step 3, but open to other suggestions. This could be done through some secondary type of DLL reference that couldn't be directly bound to.

I'm concerned we will hit this a lot with our internal packages as we try to remove indirect packages when we convert from packages.config to PackageReference.

VS version (if appropriate): 2017.2 and 2017.3

Sample project attached in broken state having updated PackB to 1.0.1
IndirectPackageReferences.zip

[mungojam]

Moved from nuget as it is perhaps more a compiler issue

[gafter]

The proposed warning could be perceived as a breaking change, and a user-unfriendly one at that.

[mungojam]

That's true, but it seems like the runtime dangers make it worth fixing somehow. I guess there are some valid times that you'd want to hide the warning as a developer if you were referencing a meta package that just brings in other packages. Or is there another solution? How about when checking bindings when building the top level application? I thought that it used to do that.

[mungojam]

Happy to help with the clarity aspect of the issue. It's a very real issue for us as we have a large number of internal packages that sometimes go 4 levels deep in the tree. I'm reluctant to migrate them to .net standard if it might lead to runtime exceptions creeping in later on if we refactor our 'Package B's in future.

We'd happily add an msbuild flag to make it strict about directly consuming transitive dependencies or to enforce a check of the bindings in AppA.

Just spotted that I left the attached solution in a funny state. ConsoleApp2 is 'App1', ConsoleApp1 can be ignored. I'll tidy up and put source on GitHub instead later on.

[mungojam]

I've been thinking about this some more. I think the change that has effectively happened is that the published API of PackB is no longer just the classes and methods that it has, but also the packages that it depends on.

We currently stick rigidly to semantic versioning, which protects us from MissingMethodExceptions. If there is no change in the new world, we need to make sure we always update the major version if we ever remove a package dependency, which is not ideal, but maybe workable.

I think that it is worse than that though. I suspect we would have to update the major version any time that we just update any package dependencies, because consumers of our package might be implicitly depending on dependencies of those lower packages that have now been removed. We generally did that before anyway but that was mainly to avoid compile-time errors so wasn't as essential as it would be now.

[mungojam]

I can how it's tricky to resolve my issue since people would expect that if PackB returns a class from PackA, you wouldn't want to have to add an explicit package reference to PackA to be able to read the classes properties or use its methods.

Perhaps it's something that tools like resharper can pick up on, if you are constructing a class or calling a static method from a package that you don't explicitly reference as a package.

[mungojam]

I have been experimenting with PackageReference flags and they seem to almost solve this but not quite. I expect there is a non-breaking change that could resolve this. I think the ideal would be a way for PackB to reference PackA such that the API of PackA was not exported to anything that just pulls in PackB (e.g. PackC). But the PackA DLLs would still come along and be deployed in any up-level project bin folder.

I had thought that maybe in PackB project, on the PackA reference, if I added 'Compile' to PrivateAssets, that would achieve what I wanted. But what happens then is that PackA doesn't come along at all so the console app fails when doing any call that needs PackA.

I think the underlying issue is covered in this comment:
slightly edited:

PrivateAssets=Compile works, at pack time it is converted into Exclude=Compile since there is no concept of private assets/build time dependencies for something after it has been built.

[mungojam]

Now working with it on GitHub: https://github.com/mungojam/TransitiveReferenceFun

[mungojam]

There has been some discussion about this on this comment stream

It would be good if there was a way to help us in using PrivateAssets="compile;contentfiles;analyzers;build" for all package references except for those that are actually exposed by our project. Or just default to this for everything and then flag on build when something needs to be exported.

It would be a pretty strict setting, but that's the default in languages like R where referenced libraries have to be exported explicitly.