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

Add tests cover using statement difference between mcs and csc. #399

Conversation

mrvoorhe
Copy link
Contributor

@marek-safar I could use your opinion on whether or not you think this is a bug in the linker.

This PR contains just tests to reproduce what I think is a bug. I did not implement a fix yet. I wanted to confirm whether or not you think it is a bug before I spend time on a fix.

AssemblyOnlyUsedByUsingWithCsc is the test that currently fails and I think it should pass.

What it does is

  • Compile an assembly called library.dll

  • Compile an assembly called copied.dll that makes use of library.dll with only the following code

// This is what triggers the behavior difference between Roslyn and mcs.  Roslyn will keep the reference
// to this assembly because of this whereas mcs will not
using ImportantForBug = Mono.Linker.Tests.Cases.References.Dependencies.AssemblyOnlyUsedByUsing_Lib;

And copied.dll is setup to have AssemblyAction.Copy

And most importantly, compile copied.dll with csc

Note that csc will include a reference to library.dll in copied.dll where as mcs will not.

AssemblyOnlyUsedByUsingWithCsc fails because I have an assert expecting that library.dll survives stripping. I think this is a valid assertion given that copied.dll references library.dll and copied.dll had AssemblyAction.Copy.

It seems to me that there is a bug in that references of an assembly with AssemblyAction.Copy should always exist after stripping, even if they've been stripped down to empty assemblies. Would you agree? What are your thoughts?

Note that I added the test AssemblyOnlyUsedByUsingWithMcs just to show what happens with mcs. This test passes.

@marek-safar
Copy link
Contributor

This looks to me like csc bug (will report that to get the consensus though).

@mrvoorhe
Copy link
Contributor Author

mrvoorhe commented Dec 6, 2018

@marek-safar looks like the roslyn issue was closed as by design dotnet/roslyn#31192 (comment), shall I proceed to investigate a fix?

@marek-safar
Copy link
Contributor

@mrvoorhe please do

@mrvoorhe
Copy link
Contributor Author

I took a brief look at trying to detect the using usage but I didn't see anything in the IL that would even let me detect this scenario.

The scenario where this was causing us problems has gone away due to some other unrelated changes. So as long as w/e the linker does results in references that exist on disk everything is fine.

It looks like SweepStep will already remove all references to assemblies that are deleted, regardless of what the assembly action was. I will update the expected behavior when csc is used to match the final output of when mcsis used. I will also add a new test framework attribute to assert the references on the assembly just to be extra certain that the reference tolibraryis removed from assemblies that haveAssemblyAction.Copy`.

Add new [KeptReferencesInAssembly] attribute for asserting the expected
references in an assembly
@mrvoorhe mrvoorhe force-pushed the master-bug-with-csc-behavior-quirk branch from c524601 to ad0cdf6 Compare January 31, 2019 16:18
@mrvoorhe
Copy link
Contributor Author

@marek-safar PR is ready for review. It's now a test only change as the current behavior of the linker is correct

@mrvoorhe mrvoorhe changed the title WIP: Add tests to show possible bug handling csc behavior quirk Add tests cover using statement difference between mcs and csc. Jan 31, 2019
@marek-safar marek-safar merged commit 1195fea into dotnet:master Feb 1, 2019
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

Successfully merging this pull request may close these issues.

2 participants