Use the full file path as the target path when it's missing

[davidwengier]

This allows projects that don't use the Razor SDK (eg, #12332) to have some kind of tooling experience. They won't have a perfect one, because there is no guarantee that the code the generator produces will compile (and in the linked issue, it doesn't), but it will produce something and will have unique hint paths etc.

Projects can essentially "enable tooling" by putting this in their project file:

	<ItemGroup Condition="'$(DesignTimeBuild)' == 'true'">
		<!-- 
			The cohosting editor uses the Razor source generator for VS functionality, so we have to make sure
			it can see the .cshtml files.
		-->
		<AdditionalFiles Include="**\*.cshtml" />

		<!--
			Now we have to make sure the generator is actually referenced. Luckily the Razor tooling will automatically
			redirect any reference to it, to the right location of the file that ships with VS, so we don't have to care
			where it actually is.
		-->
		<Analyzer Include="Microsoft.CodeAnalysis.Razor.Compiler.dll" />

		<!--
			Make sure the source generator knows where the project is, for computing target paths.
		-->
		<CompilerVisibleProperty Include="MSBuildProjectDirectory" />
	</ItemGroup>

Source generator changes:

• If there is a target path supplied, use it
• If there is no target path supplied, but we know the project path, compute a target path
• If there is no target path supplied, use the whole file path

Tooling changes:

• Instead of trying to have an algorithm perform similar logic and guess what the source generator would have done, we just look through the source generated documents and find a match. Full path matches always win, otherwise project relative, being careful to watch out for duplicates.

The "use the whole file path" change is arguably separate, and it is TBD if it is the way we want to go. If we don't do that, it just makes the compiler visible property part of the above required.

[davidwengier]

Drawing board. Hint names need to match target paths...

[DustinCampbell]

I'd prefer to see a more deterministic solution for miscellaneous files rather than just trying the physical file path if there's no target path. In general, the physical file path is probably going to cause issues when used as the target path.

Here's how I imagine in work:

• Is this file associated with a project?

  • Is the target file path available? If so, use it! We're done.
  • If the target path isn't available and the physical file path is within the project cone, construct a target path by computing the relative physical file path within the project.
  • If the physical file path is outside of the project cone, something is wrong. Report a warning and treat the file as a miscellaneous file as below.

• Is this a miscellaneous file? (i.e. it's not associated with a project.)

  • Each miscellaneous file should belong to its own project.
  • The target path should be the file name, so it's considered to be in the root of the project.

At least, that's how I think it should basically work, and we can make refinements around that. What do you think?

[davidwengier]

@DustinCampbell your suggestions match my desires perfectly, but AFAIK the source generator is entirely unable to answer either of those two top level questions :)

[DustinCampbell]

@DustinCampbell your suggestions match my desires perfectly, but AFAIK the source generator is entirely unable to answer either of those two top level questions :)

What changes are needed in Roslyn to make this work?

[davidwengier]

There definitely should be a larger discussion about TargetPaths I think (and I think @chsienki might have started playing with just hashing full paths?), and giving a source generator access to project path is definitely one potential approach.

In general, the physical file path is probably going to cause issues when used as the target path.

I'm curious for your thoughts on this. I've been thinking that using the full path is actually what I should have done in misc files to begin with, to "guarantee" separation of documents. "One misc files project with lots of random files" vs 'lots of misc files projects with one file each" seems like an implementation detail of Roslyn that the source generator should probably be immune to (though happens to make basic functionality in non-Razor-SDK projects easier, so I'm definitely biased in that opinion 😊)

Either way, just to be clear on my intentions, I'm not aiming to "fix" target paths completely with this PR, just unblock a subset of users who are seeing crashes. If I can make a, hopefully pretty minor, change in tooling to not be so dependent on knowledge of the TargetPath algorithm, that should be all it takes. It's only a couple of features that need this, as once we're in OOP it's not a problem. It won't make those projects work perfectly or anything, just make it not crash out.

[DustinCampbell]

In general, the physical file path is probably going to cause issues when used as the target path.

I'm curious for your thoughts on this. I've been thinking that using the full path is actually what I should have done in misc files to begin with, to "guarantee" separation of documents. "One misc files project with lots of random files" vs 'lots of misc files projects with one file each" seems like an implementation detail of Roslyn that the source generator should probably be immune to (though happens to make basic functionality in non-Razor-SDK projects easier, so I'm definitely biased in that opinion 😊)

The compiler will throw if a physical file path is provided for the target path and it falls outside of the project's physical file path. This is because target paths that are not within the project cone can break the view engine at runtime. We've gotten away with cheating in tooling if the compiler got the right target path during compilation. However, now that it's essentially all the same, I don't think we play as "fast and loose" with target paths.

[davidwengier]

The compiler will throw if a physical file path is provided for the target path and it falls outside of the project's physical file path

Could you point to that? I know that this exception exists, but I don't think that's relevant here because, at least to my limited understanding, the source generator doesn't use that system. It would only be hit by things in legacy or non-cohost tooling, where the target path comes via tooling. This code change only affects the generator, and only when the globalconfig doesn't exist, and therefore there is no target path.

If the source generator knew to throw for a target path that wasn't in the project cone, then that means it knows where the project cone is, and I could compute a "proper" target path, but as mentioned above I don't think that's possible.

[DustinCampbell]

The compiler will throw if a physical file path is provided for the target path and it falls outside of the project's physical file path

Could you point to that? I know that this exception exists, but I don't think that's relevant here because, at least to my limited understanding, the source generator doesn't use that system. It would only be hit by things in legacy or non-cohost tooling, where the target path comes via tooling. This code change only affects the generator, and only when the globalconfig doesn't exist, and therefore there is no target path.

If the source generator knew to throw for a target path that wasn't in the project cone, then that means it knows where the project cone is, and I could compute a "proper" target path, but as mentioned above I don't think that's possible.

You're right. The source generator uses VirtualRazorProjectFileSystem, which doesn't throw like DefaultRazorProjectFileSystem. That's definitely what I was thinking of.

Is there any technical reason why the source generator couldn't know the project file path? After all, we're getting the target path by flowing it through from the project file aren't we?

[davidwengier]

I don't think the C# compiler knows the project file path, as its not part of the csc args, but I'm getting well out of my depth here.

[DustinCampbell]

I don't think the C# compiler knows the project file path, as its not part of the csc args, but I'm getting well out of my depth here.

Sure, but the C# compiler wouldn't know the target path either. I'm assuming the target path flows from targets that attach it as metadata to the additional file items. Then, that metadata makes its way (somehow) into the AnalyzerConfigOptions. I'm looking this blob of code:

razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.RazorProviders.cs

Lines 81 to 101 in e6d2b8b

	if (options.TryGetValue("build_metadata.AdditionalFiles.TargetPath", out var encodedRelativePath))
	{
	// TargetPath is optional, but must have a value if provided.
	if (string.IsNullOrWhiteSpace(encodedRelativePath))
	{
	var diagnostic = Diagnostic.Create(
	RazorDiagnostics.TargetPathNotProvided,
	Location.None,
	additionalText.Path);
	return (null, diagnostic);
	}
	relativePath = Encoding.UTF8.GetString(Convert.FromBase64String(encodedRelativePath));
	}
	else
	{
	// If the TargetPath is not provided, we effectively assume its in the root of the project.
	relativePath = Path.GetFileName(additionalText.Path);
	}
	options.TryGetValue("build_metadata.AdditionalFiles.CssScope", out var cssScope);

If that all comes from targets, I can't see why we couldn't pass whatever we need.

[davidwengier]

Oh, yes, that all comes from targets in the Razor SDK. We could absolutely pass along the project file path with that info, and calculate target path in the generator and not the SDK. Sadly that doesn't help this situation though, because this fix only applies when the SDK isn't being used, so the project file path wouldn't be passed along anyway :)

Adding something like that to the base .NET SDK, or Roslyn itself, would certainly not getting any arguments from me, but again, this PR is just intended as a tactical fix to have users not hit an exception.

[davidwengier]

Look what I found!

https://github.com/dotnet/sdk/blob/ffaba7534225db5528a00d45be71127f3f871843/src/RazorSdk/Targets/Microsoft.NET.Sdk.Razor.SourceGenerators.targets#L54

I'm gonna try putting in fallback support for that (if TargetPath isn't specified), and seeing if that can unblock things, by just having people include that one line in their non-Razor-SDK projects. I think it should work great.

[davidwengier]

Okay, updated this. I've left the "use full file path as target path" in for now, though it's no longer load bearing. Planning to discuss at the compiler working group tomorrow.

[DustinCampbell]

This solution is so much better! Thanks for digging deeper into this.

[davidwengier]

@chsienki Please take a look at the compiler change here, and see what you think. I've updated this now so that the target path diagnostic is only reported if we can't compute a fallback, which is likely never. I still maintain it doesn't make sense to stop generating code when only reporting a warning, but I haven't changed that here.

With this final change, this means the project from #12331 has a full tooling experience, and is potentially is resolved with the addition of the following to the project file:

	<ItemGroup Condition="'$(DesignTimeBuild)' == 'true'">
		<Analyzer Include="Microsoft.CodeAnalysis.Razor.Compiler.dll" />
		<CompilerVisibleProperty Include="MSBuildProjectDirectory" />
	</ItemGroup>

Unlike the issue I mentioned in the description of the PR, the Umbraco project is at least targeting modern .NET. I'm leaving the issue open though as I think it would still be good for us to understand why those extra things are needed, and if it's an issue we can/should address in the SDK. Plus I don't know if the breakpoints actually get hit, because I don't have an Umbraco instance running to actually try with (nor would I know what to do with it anyway 😛)

FYI @DustinCampbell for the commit after your approval.

[chsienki]

I'm fine leaving this the way it is for now, but I actually think given that we have the project file anyway, we should just ditch the target path calculation in MSBuild and just do it here always.

My suspicion for why it was done like this is that the old pre-SG way of compiling had logic in MSBuild for target path calculation, and the authors didn't want to have a duplicate that could get out of sync.

[chsienki]

Filed #12390