Non-Resx Embedded Resources get mangled names, possible name collisions

[Anipik]

I was able to create a small repro

the csproj looks like this

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <EmbeddedResource Include="MyResource.bmp" WithCulture="false" Type="Non-Resx" />
  </ItemGroup>
</Project>

Program.cs

namespace resourceName
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine(MyResource.Get()?.Length ?? -1);
        }
    }
}

MyResource.cs

namespace resourceName
{
    public static class MyResource
    {
        public static Stream Get() => typeof(MyResource).Assembly.GetManifestResourceStream($"{nameof(resourceName)}.{nameof(MyResource)}.bmp");
    }
}

Running this code under preview 7 gives output as

C:\git\scratch\resourceName>dotnet.exe run
485514

where as running this code under preview 9 gives output as

C:\git\corefx>dotnet c:\git\scratch\resourceName\bin\Debug\netcoreapp3.0\resourceName.dll
-1

I checked dlls
the resource name include in .dll in previe9 is resourceName.MyResource where as under preview 7 is resourceName.MyResource.bmp

I hit this error while building corefx with preview9 for testData.resources file.

This works fine with 5.0 as well.

cc @ericstj @danmosemsft @livarcocc

[ericstj]

@Anipik can you zip up the project and add it to the issue? Does setting EmbeddedResourceUseDependentUponConvention = false also fix it?

[Anipik]

yes setting EmbeddedResourceUseDependentUponConvention to false fixes it for preview9
MyResource.zip

[ericstj]

Looks like this was regressed with #4597. @benvillalobos @rainersigwald

I wonder if the DependentUpon heuristic should be scoped a little?

[nguerrera]

We probably should have only applied the convention to .resx resources.

[nguerrera]

If we want to change this, we should do it ideally quickly as it would be breaking to change the resource names again. I think we're too late for 3.0 which makes this difficult. :(

[nguerrera]

Restricting to resx makes sense because that is the case that would have been persisted with DependentUpon in a classic project. I'm not aware of anything that would have put DependentUpon for non-resx in your project unless you did it manually so I don't think it should have been part of the convention to do so.

[nguerrera]

A very similar repro could be constructed for a resx though, in some cases the convention will pick a different name than earlier previews (that's the point of the change after all) so it is to be expected that some projects will be broken by the convention, this is why I insisted on scoping the default to TFM so that it would only break 3.0 preview projects. But it is unfortunate that non-resx is affected at all.

[nguerrera]

It turns out that this changes the extension from .bmp or .jpg or whatever else non-resx to blank. So even if the source file found by convention has first class RootNamespace.Folder.Filename, the manifest resource will not match what you would get without the inferred DependentUpon

[nguerrera]

I imagine there will also be build errors if you have say Foo.txt and Foo.bmp as embedded resources next to Foo.cs. Without preserving the extension, this will cause duplicate resource names.

[rainersigwald]

Looks like there's another related failure mode here:

error CS1508: Resource identifier 'C' has already been used in this assembly"

From https://dev.azure.com/dnceng/public/_build/results?buildId=361408&view=results (thanks, @ViktorHofer).

We'll have to dig in, but I suspect this is the thing Nick predicted

I imagine there will also be build errors if you have say Foo.txt and Foo.bmp as embedded resources next to Foo.cs. Without preserving the extension, this will cause duplicate resource names.

[ericstj]

That CSC failure is because these source files have a class named C in them:
https://github.com/dotnet/corefx/blob/b020d0033ffe755f779c564186fc2d98b0af31cc/src/System.Reflection.Metadata/tests/Resources/Misc/Members.cs#L7
https://github.com/dotnet/corefx/blob/b020d0033ffe755f779c564186fc2d98b0af31cc/src/System.Reflection.Metadata/tests/Resources/Misc/Debug.cs#L7

And they have DLLs embedded with the same name as the source file:
https://github.com/dotnet/corefx/tree/master/src/System.Reflection.Metadata/tests/Resources/Misc

[nguerrera]

Alright, I think we need to remove the convention from affecting non-resx resources. It's becoming pretty clear to me that the odds of wanting the convention to apply (even if it preserved extension) are close to zero. I suspect eveyone in this case is going to disable the convention.

This is strictly a breaking change from 3.0 /16.3 GA (assuming we cannot get this fix into that, and I don't think we can), but I will bet that most of the people who hit this will disable the convention entirely and therefore not get re-broken when it gets less aggressive.

I would consider taking this in 16.0.x/3.0.1xx servicing as quickly as feasible so that the window of time to take a dependency on this odd, non-deliberate fallout from the change is as short as possible.

@livarcocc @ericstj @rainersigwald Do you agree?

[rainersigwald]

restrict convention to .resx: yes

service for it: probably

[nguerrera]

Maybe 16.4/3.1 being not too far out is good enough. If we think most people with this code pattern will have to disable the convention, then it's not really breaking, and will help others who only move to 3.x when it goes LTS.

[nguerrera]

Just a note that there is another hit count on this from a customer in a mail thread.

[nguerrera]

HitCount++

https://twitter.com/simoncropp/status/1175385910919090176?s=21

This one is the other duplicate name case I predicted.

[panost]

Got hit by this in all my projects, after upgrading to VS 16.3
The error was CS1508: The resource identifier ... has already been used in this assembly
I have resources that generated on build and somehow msbuild made all the wrong assumptions, on how to name them. I got a lot of lines in CreateCSharpManifestResourceName like this
Resource file 'blah..' gets manifest resource name 'wrong_name_blah'.

Thankfully the EmbeddedResourceUseDependentUponConvention=false workaround works, but it took me 3 missing hours to figure what happen.
Sorry for the long post, I am actually trying to make it google searchable, if anyone has the same problem

[SimonCropp]

@panost i feel your pain. i burnt 2 hours :)

[rainersigwald]

Sorry for the troubles, folks. This issue is now listed as a .NET Core 3.0 known issue to hopefully improve visibility.

[rainersigwald]

Fixed for 16.3 servicing (and 16.4) by #4764.

[benvillalobos]

For folks who stumble onto this closed issue, refer to this dotnet docs issue for a detailed explanation on how manifest resource naming works now.