by

Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 07:11
    dotnet-maestro[bot] commented #44543
  • 07:06
    dotnet-maestro[bot] commented #44543
  • 07:01
    dotnet-maestro[bot] commented #44543
  • 06:56
    dotnet-maestro[bot] commented #44543
  • 06:51
    dotnet-maestro[bot] commented #44543
  • 06:46
    dotnet-maestro[bot] commented #44543
  • 06:44
    cston milestoned #44912
  • 06:44
    cston labeled #44912
  • 06:44
    cston labeled #44912
  • 06:43
    cston review_requested #44912
  • 06:43
    cston opened #44912
  • 06:41
    dotnet-maestro[bot] commented #44543
  • 06:41
    jcouv commented #44911
  • 06:40
    jcouv commented #44911
  • 06:36
    dotnet-maestro[bot] commented #44543
  • 06:31
    dotnet-maestro[bot] commented #44543
  • 06:26
    dotnet-maestro[bot] commented #44543
  • 06:21
    dotnet-maestro[bot] commented #44543
  • 06:18
    buster95 commented #25401
  • 06:17
    buster95 commented #25401
Joseph Musser
@jnm2
This is in a world where view models are agnostic to the presence or absence of the view
Antony Male
@canton7
@jnm2 Think vim or sed
stefanloerwald
@stefanloerwald
So that test would test if there are the expected classes (e.g. controllers), but not more. Sounds like what a German would call "von hinten durch die Brust ins Auge" [from behind, through the chest, into the eye]. Why not simply register the classes then? :-D
Joseph Musser
@jnm2
Because I'd have to maintain a parallel list?
Seems like work I could do without, but Yair's saying differently and I'm listening.
stefanloerwald
@stefanloerwald
Because we're arguing that "magic autoregistration" of types is nonintuitive, and adding them manually would be more intuitive (even if slightly more effort, but hey, there could be an analyzer saying "hey, you forgot this controller!"). Having a test for the magic is possible, but I would prefer a different design without that magic.
Joseph Musser
@jnm2
Dynamic location of types by name or interface or attribute is fairly common. Take NUnit or xUnit, or Roslyn analyzers, or plugins of any sort.
HaloFour
@HaloFour
I'm so used to the magic at this point that it doesn't bother me that just adding a class will affect behavior, but I can understand the desire to want to be more explicit.
@YairHalberstadt IMO the wording of the question might trip folks up
stefanloerwald
@stefanloerwald
I think there's a place for that, absolutely (e.g. I don't want the boilerplate code for test discovery).
For roslyn it's for discovery of analyzers, I suppose?
Then I don't mind that, because I usually explicitely add the analyzers as packages.
Yair Halberstadt
@YairHalberstadt
@jnm2
Agreed for plugins, because I don't see an alternative, and tests because the benerfits are so huge and it's so common
I'm talking about a random interface in a random codebase :-)

But you do lose something in plugins.

You can't define a bunch of analyzer in one package, and then reexport a subset in a different package (I assume)

Manish Vasani
@mavasani

@stefanloerwald

Hi, I'm struggling with getting my code fix provider visible on the diagnostics outside of the unit tests. I correctly get the diagnostics (warnings) in my production code, but I don't get the code fix offered on the class. Entire code for the code fix provider is here: https://pastebin.com/HNPTrnMp What might the issue be? Any common pitfalls?
This is an analyzer deployed as nuget package, not vsix.

Is your UnusedInternalClassesAnalyzer implemented with a compilation end action? If so, diagnostics reported from these actions are not fixable from lightbulb. Compilation end diagnostics need entire project analysis to complete to be reported, which is not feasible from a performance standpoint during a Ctrl + Dot operation. Compilation end diagnostics are effectively build-only diagnostics.

Joseph Musser
@jnm2
@YairHalberstadt @stefanloerwald So from my point of view the general concept is a familiar one that folks recognize immediately once they see it.
Yair Halberstadt
@YairHalberstadt
Well how would you see it?
stefanloerwald
@stefanloerwald
@mavasani that's exactly it. Ouch. That's quite sad. Because I don't see any way to make that analysis a non-compilation-end-action one...
Yair Halberstadt
@YairHalberstadt
Presumably the code fix doesn't do much?
stefanloerwald
@stefanloerwald
The code fix only removes the unused code. It's not the end of the world that this is a manual step.
Yair Halberstadt
@YairHalberstadt
That's what I thought. Does it detect cycles by the way?
Or special case Main?
stefanloerwald
@stefanloerwald
You mean that it detects whether the only references are from within the class itself? That it does
I didn't think of Main :-D
Yair Halberstadt
@YairHalberstadt
I mean A references B which references A?
It's probably not a good idea to detect them since chance of error goes up. If even one of those classes is used by reflection, the entire graph is used.
stefanloerwald
@stefanloerwald
No, that I don't do yet. Interesting point.
I think it could be an interesting addition.
Reflection of course is the killer of this analyzer. That's of course only if the absolute only way that class is used is through reflection. As soon as there is at least one proper use of the class, then it's all fine.
stefanloerwald
@stefanloerwald
(which can be anything. typeof(T), nameof(T), private T field; ....)
Manish Vasani
@mavasani
@stefanloerwald Note that this/similar analyzer already exists in Microsoft.CodeAnalysis.FxCopAnalyzers NuGet package: https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUninstantiatedInternalClasses.cs
I think it handles all the special cases you mentioned here
stefanloerwald
@stefanloerwald
That would explain why it's quite a bit more code than I have written :-P thanks for letting me know. I guess I'll ditch my implementation then. But good to learn from. Thanks
Joseph Musser
@jnm2
@stefanloerwald Also, be aware of dotnet/runtime#33861 (+ dotnet/runtime#36708)

(which can be anything. typeof(T), nameof(T), private T field; ....)

Would you count crefs?

Joseph Musser
@jnm2
This is really nice:
  • Quick Info now displays the diagnostic ID along with a help link where you can easily navigate to our documentation to learn more about warnings and errors in your code.
Joe4evr
@Joe4evr
ooooh, nice indeed
Joseph Musser
@jnm2
Is there a way to go from an IFieldSymbol to an IOperation which describes the field's initializer?
CyrusNajmabadi
@CyrusNajmabadi
good question
@mavasani ?
Joseph Musser
@jnm2
Now I'm thinking no because an IFieldSymbol didn't necessarily come from a semantic model
CyrusNajmabadi
@CyrusNajmabadi
well, it would have to be a source ymbol
but seems reasonable
Joseph Musser
@jnm2
Higher-level, I want a to make a helper method that gives me an IFieldInitializerOperation for a specified metadata type name and field name.
I built it to return an IFieldSymbol and realized that it was probably a dead end.
Seemed at first like it might compose fine.
Joseph Musser
@jnm2

This is for unit tests. Current helper:

public static IFieldInitializerOperation GetFieldInitializerOperation(Type declaringType, string fieldName)
{
    var compilation = GetCompilationContaining(declaringType);
    var fieldSymbol = compilation.GetTypeByMetadataName(declaringType.FullName).GetMembers(fieldName).OfType<IFieldSymbol>().Single();

    var initializerSyntax = fieldSymbol.DeclaringSyntaxReferences
        .Select(r => ((VariableDeclaratorSyntax)r.GetSyntaxAsync().GetAwaiter().GetResult()).Initializer)
        .SingleOrDefault(initializer => initializer is { });

    if (initializerSyntax is null) return null;

    var semanticModel = compilation.GetSemanticModel(initializerSyntax.SyntaxTree, ignoreAccessibility: false);

    return (IFieldInitializerOperation)semanticModel.GetOperation(initializerSyntax);
}

Guessing it's a bad idea to get the semantic model more than once in a test run without holding onto it

Wait, IFieldInitializerOperation has .SemanticModel, so it stays alive :+1:
Joseph Musser
@jnm2
(I'm aware that I'm blocking instead of awaiting. This code doesn't ship, so if test runs get too slow, we can start caring.)