Closed Pzixel closed 5 years ago
Can you tell what is that known error?
I've updated the build task recently, but I think I kept the silent warning behavior. I may be wrong, or we need to change the behavior.
I'm talking about this project: https://github.com/Pzixel/Solidity.Roslyn
It generates C# wrappers for solidity types so they are callable from C#. It's a known issue that user may have invalid solidity souce file (I missed an identifier in the example above). In this case solidity compiler throws an error and it's clear that I cannot generate C# wrappers, because I should get AST from solidity compiler, but instead I get bunch of errors.
In this case I just want to stop the compilation process and show errors to the user for he could fix them.
I might've introduced that behavior. If so, it originates in https://github.com/AArnott/CodeGeneration.Roslyn/pull/83/files#diff-94e71052ac3ede7cfac37443a34398a9
I'm not sure now if the pre-change behavior also resulted in warnings. It's possible it returned errors. I'll investigate and push a PR with a fix, but it might take me some time.
The fix most probably requires removal of ContinueOnError
attribute at https://github.com/AArnott/CodeGeneration.Roslyn/pull/83/files#diff-94e71052ac3ede7cfac37443a34398a9R38
I've browsed through error handling in the tool. Currently it's a chaotic mashup of direct-console logging, IProgress reporting and a custom Logger class. I do believe that a refactor is in order.
I think that IProgress should stay, as designed, an output channel for generators. I am not sure what the role of a custom Logger is, but it is a public interface that allows a much simpler output, directly to console but with the Console hidden within implementation.
Then there are instances of direct Console usage, like:
Program.cs error handling (and it seems it was me who changed it from Logger to Console - I think it was because the original behavior caused a lot of duplicated error reporting, and I tried to de-duplicate it at least in Visual Studio Error List). https://github.com/AArnott/CodeGeneration.Roslyn/blob/38605a454be13f09cb13a8ee944bf7c318510e9d/src/CodeGeneration.Roslyn.Tool/Program.cs#L38-L42 https://github.com/AArnott/CodeGeneration.Roslyn/blob/38605a454be13f09cb13a8ee944bf7c318510e9d/src/CodeGeneration.Roslyn.Tool/Program.cs#L44-L48 https://github.com/AArnott/CodeGeneration.Roslyn/blob/38605a454be13f09cb13a8ee944bf7c318510e9d/src/CodeGeneration.Roslyn.Tool/Program.cs#L66-L71
In CompilationGenerator.cs there is an additional log of the error into a Console. https://github.com/AArnott/CodeGeneration.Roslyn/blob/38605a454be13f09cb13a8ee944bf7c318510e9d/src/CodeGeneration.Roslyn/CompilationGenerator.cs#L245
And of course in the Logger: https://github.com/AArnott/CodeGeneration.Roslyn/blob/38605a454be13f09cb13a8ee944bf7c318510e9d/src/CodeGeneration.Roslyn/Logger.cs#L59-L70
I think the logger was initial created by me. The previous console output was not shown in the build output with default verbosity. If I remembered correctly.
IIRC IProgress is always null since it's never gets assigned. I could be wrong, but when I tried to use it I was always gettin NRE.
https://github.com/Pzixel/RemoteClient.Roslyn/blob/master/RemoteClient.Roslyn/RemoteClientGenerator.cs#L34 - as you can see, I use null propagation becaus IProgress may be missing. It's really not convinient, but I didn't come to the better solution at the time.
Another point is that OnDiagnosticProgress just prints the diagnostics where it probably should do something like
private static void OnDiagnosticProgress(Diagnostic diagnostic)
{
if (diagnostic.Severity == DiagnosticSeverity.Error)
{
Console.Error.WriteLine(diagnostic.ToString());
}
Console.WriteLine(diagnostic.ToString());
}
IProgress was fixed with PR #45 and is now working:
It might be a good idea to print the diagnostic on error output for error level.
I think the inconsistency is the result of the evolution of the generator initially being T4, then an MSBuild task, and now a dotnet CLI tool. Ideally, when run in an MSBuild invocation, the errors/warnings are reported via the MSBuild reporting mechanism for such. But I don't know how we can properly pipe that through to the task. But MSBuild has excellent built-in parsing support for invoked tools, so long as they follow a standard format for warning/error output messages. Can we make sure that we follow that pattern, and then adjust the msbuild target that invokes our tool to set the option that MSBuild should parse the output, looking for errors/warnings?
I think the Logger already does that here:
The only problem is that it will parse a new error/warning for each line of the message logged.
Also, isn't that "parsing" the default behavior of MSBuild?
Yes, that may all be set up. I was just speaking from memory and speculating based on what I'm seeing here.
Marking as bug because I believe there was a change from the failure to generate being an error to a warning. It should be probably reverted, because using generators should be conscious, and current behavior doesn't work well for users.
Any progress on that?
I currently have my generator fail for some known reasons. But instead of showing a nice error it just silently throws a warning, and then some depended code fails. It doesn't seem right: