Put generated files to disk:#47047
Conversation
3b98f68 to
c7a7c37
Compare
c7a7c37 to
ae65037
Compare
|
@dotnet/roslyn-compiler for review please |
|
:( |
|
@jmarolf Why the sad face? |
|
@jmarolf if you have constructive feedback about this PR I'd be interested in hearing about it. A simple frowny face though doesn't really move the conversation forward, it just reduces the momentum of PRs and causes confusion. This is a long planned feature for source generators backed by customer scenarios. Specifically customers want the ability to see the actual generated source on disk and the compiler is providing that ability. |
Sorry, there isn't a link to a bug in this PR and the description isn't sufficient to understand where the customer scenario is coming form. My sad face should be interpreted as "this is unfortunate but I am not going to block this PR if there are good reasons™ we need to do it" I have seen almost all source generator authors try and mess with file IO. the fact that we do not generate files has led a lot of them to come to forums and ask why and we explain "your source generator should not be doing file IO". Our current design appears (anecdotally) to guide the user into the pit of success of allowing file operations to be tracked by MSBuild. Once this is checked in source generator authors will check for the existence of generated files, most likely do IO to read them, and then decide if they need to regenerate. This is the pattern most authors first try before running into a wall. Without any details about why this is being done I can only speculate but if this is about either
I would like us to take a different approach. Neither of those two cases screams that we need this functionality and I would prefer our current approach of not making file IO an easy thing for source generator authors to do. |
|
@jmarolf There isn't an issue, but it was always part of the design to (optionally) persist generated files to disk: https://github.com/dotnet/roslyn/blob/master/docs/features/source-generators.md#output-files Note that we don't use them for debugging, or build correctness (we consulted with the MSBuild team too while designing this). Yes it potentially opens up an avenue for authors to attempt to 'optimize' their generator by not generating any source, but there's nothing stopping them from doing that themselves anyway, so I don't really think we're opening up a pit of failure with this change. Personally, I wish we'd never write anything at all (hence why the design is you can disable it, and we work entirely off on in-memory representations), but we heard from multiple customers during the design phase that they wanted this feature; we've deliberately waited to implement it later on to see if it was actually needed, and it's been one of the most vocal pieces of feedback we've heard from customers (both authors and consumers). |
I don't see how that is related to this change at all though. This is about the compilation process choosing to emit extra artifacts that we control to disk. Not morally different than the XML doc file, PDB, etc ... It doesn't have any relation to SG authors trying to read / write files in their code. |
There was a problem hiding this comment.
Why ToImmutableArray vs. say ToList? This doesn't ever appear to be used as an ImmutableArray<T> hence seems like this is just adding some extra overhead.
There was a problem hiding this comment.
I needed the length; I am a cargo cult programmer so I used ImmutableArray because we use it everywhere else instead of lists.
More seriously, is there a perf preference between list and regular array? My intuition here is to use .ToArray() as we only need the length and to iterate over it.
There was a problem hiding this comment.
On framework ToList is generally faster than ToArray due to having one less allocation. On .NET Core it's somewhat the opposite because they use better builders (got schooled on that fix on twitter recently). Would probably go with ToList for now.
There was a problem hiding this comment.
Segmented arrays would be the best case scenario here. I've completed the implementation of SegmentedArray<T> but I'm still working through the details to get SegmentedList<T> in place.
Its only related in that before source generators were "stateless". You could no know as a source generator if you had run previously. It was not possible with out pretty unusual hacks. Now it will be possible for source generator authors to pass this flag in and use previous build artifacts to know if they've run. It's a very minor point (thus the very minor :( face). |
There was a problem hiding this comment.
The help string (for csc --help) should be updated to document new option. #Resolved
There was a problem hiding this comment.
RoslynString [](start = 32, length = 12)
nit: I think string.IsNullOrWhiteSpace would do (RoslynString was there to add nullability annotations, but shouldn't be necessary now) #Resolved
There was a problem hiding this comment.
ERR_SwitchNeedsString [](start = 69, length = 21)
I couldn't find ERR_SwitchNeedsString in the added sections of the test files. Please add a test, if we don't have one. #Resolved
There was a problem hiding this comment.
GeneratedFilesDirectory [](start = 44, length = 23)
nit: there is a small inconsistency in naming between GeneratedFilesOutputPath and GeneratedFilesDirectory. Is that intentional?
There was a problem hiding this comment.
We have OutputPath and OutputDirectory, so I just made it match the same pattern. No objection to unifying it though.
In reply to: 478710831 [](ancestors = 478710831)
There was a problem hiding this comment.
ToImmutable [](start = 83, length = 11)
Do we need to materialize array here? I think the AddRange(IEnumerable<T>) overload should work. #Resolved
There was a problem hiding this comment.
Do we have test coverage for the touched files logic? #Resolved
There was a problem hiding this comment.
disposer [](start = 46, length = 8)
nit: "ignoredDisposer"?
There was a problem hiding this comment.
There was a problem hiding this comment.
It's fine. I was thrown off, trying to find where it was used. If we supported discards in usings, we'd use a discard ;-)
There was a problem hiding this comment.
nit: just curious, how do we know that?
There was a problem hiding this comment.
Because we create them ourselves in the GeneratorDriver. If they don't have a path then something weird is going on.
In reply to: 478717222 [](ancestors = 478717222)
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 2)
That definitely concerns me. It will be abused. Could we emit to a random location (and provide a flag to have us report what that random location is)? That way the user can go and look at teh source. But they could never depend on it to store state from run to run? |
The specific design, as expressed by our customers, is to have an option to emit files to disk for inspection. This is agreed on by the people involved. I am a bit surprised at all of the push back here. There are a variety of options that can be abused by generator authors if they wish. This is just another one of them. The abuse here is no different than any other unlisted IO in the build tasks. That violates the build completely and is possible today without any flag. If your "request changes" review is because you disagree with the design then I'd ask that you remove that. This is the design, it's been this way from the start and there has been ample opporutunity to push back on it. At this point we are in the end game of the product cycle and we are attempting to deliver the existing design. We can have a separate convo about the design but we'd like to move forward with the existing design so we can deliver on our RC1 commitments. If you have feedback on the implementation more than happy to hear it and react here. |
|
Note: this currently targeting master, but should go into release\dev16.8. Its got a merge from master in the commit history, so I'll rebase and retarget before merging. |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM--had a couple comments about test code, but do not need to be addressed given the timeline.
There was a problem hiding this comment.
what effect does ToArray have here?
There was a problem hiding this comment.
Copies it, so we don't modify the collection as we're iterating it :)
There was a problem hiding this comment.
nit: is the CleanupAllGeneratedFiles call redundant given this call? (and in subsequent usages)
There was a problem hiding this comment.
Yes, actually, we don't need it, i'll make a note to clean that up later on. Thanks
63551c8 to
834a54c
Compare
There was a problem hiding this comment.
If we pass the switch more than once, will that just ignore everyone but the last?
There was a problem hiding this comment.
Yes, this is the same behavior as /out: etc.
There was a problem hiding this comment.
Why are these changing like this in this PR?
There was a problem hiding this comment.
Hmm, I think this is a mis-rebase due to the merge I had from master. Good catch, will clean up with a new rebase.
There was a problem hiding this comment.
string.Empty in earlier tests but not here?
There was a problem hiding this comment.
There's no surprises here about other things adding trees? Nothing funky on the VB side of things around My since I think that implicitly adds trees, but probably not to the arguments compilation's SyntaxTrees collection?
There was a problem hiding this comment.
Nothing funky for C#. VB and My trees is a good thought, but the generators wont get loaded in that case, so this code will never execute.
There was a problem hiding this comment.
How would we ever have an assembly with no name? I presume that means the file pathing would get screwed up since we'll have one less file component, right?
There was a problem hiding this comment.
I actually have no idea, and couldn't easily make a test that did. The file path will just be /[typename]/[hintName.cs] in those cases. So we might see some oddities in file mathching in the IDE, but it shouldn't crash and burn at least.
There was a problem hiding this comment.
Well, the concern here being the "assembly" name will then actually be the projectDirectory name and matching won't work at all.
There was a problem hiding this comment.
Yep. Basically if you somehow pass a generator from an assembly with a null name, you wont be able to look at its files in the IDE. (If someone legitimately has a use case, we can fix it later on).
There was a problem hiding this comment.
Can we remove the Path.GetFileName here and have it assert the full name, since we should know the full path that's being applied?
There was a problem hiding this comment.
Why this check? Directory.CreateDirectory() will create it for you...
There was a problem hiding this comment.
It will, but we want it to explicitly error if the directory you've given us doesn't exist. We're saying 'we'll create the subdirectories, but not the main one'.
Now, the msbuild logic will create the output directory for you if you create it, but we don't really want to be in the business of creating arbitrary directory structure inside the compiler.
There was a problem hiding this comment.
but we don't really want to be in the business of creating arbitrary directory structure inside the compiler.
I guess it seemed odd to actively not be in the business, since you're still creating all the other directories.
There was a problem hiding this comment.
Yeah, but we're creating a known set of sub-directories, not some arbitrary random path. I'm on the fence tbh, but we have tests covering the behavior so for now, #WontFix
There was a problem hiding this comment.
Will this run during a design time build? It's probably not the worst thing to be doing as there's far greater sins...
There was a problem hiding this comment.
Disabled it for design time builds.
There was a problem hiding this comment.
I'd break this "When..." sentence up into two sentences, one with the set and unset case. Otherwise it's confusing whether the "and issue a warning if true" applies to which conditional. (This read as "If A, we'll do B when A isn't true.")
There was a problem hiding this comment.
So this could only really happen if line 285 failed to run, which is only if you don't have an intermediate output path to start with I guess?
There was a problem hiding this comment.
(seems unlikely, but sure. 😄)
There was a problem hiding this comment.
Yeah, it covers the case that you don't have an intermediate directory set, and you didn't explicitly provide a generated files output.
- Add new command line param 'GeneratedFilesOut' - Route new param through tasks with defaults - Write out generated files when requested to do so - Rationalize post generation to avoid uncesseary work - Add Tests for targets and emit
- Rename MSBuild properties
- Add test to track the fact that we can overwrite files on windows in SxS cases
ac526ba to
bbc51cc
Compare
dotnet#47047 changed the logic for the file name and path we generate, but we were no longer ensuring the temporary directory existed prior to trying to create the file within it.
This no longer needed since intellisense works just fine nowadays. In addition, if persisting the sources for inspection is required, the following two MSBuild properties can be used to persist them: `EmitCompilerGeneratedFiles` and (optionally) `CompilerGeneratedFilesOutputPath`. If no custom path is provided, files will be persisted to the `$(IntermediateOutputPath)/generated/[GeneratorAssembly]/[GeneratorTypeFullName]` folder. See also dotnet/roslyn#47047.
Fixes #47211