efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Additional possible diagnostics analyzers/suppressors

Open roji opened this issue 5 years ago • 4 comments

  • [ ] N+1 detection (apparently Jetbrains are working on this).
  • [ ] Internal usage analyzer: generic type argument for generic types (e.g. in instantiation)
  • [ ] #25022
  • [x] #12012
  • [x] #30965
  • [ ] #25455
  • [ ] #27856
  • [ ] #29988
  • [ ] #11217
  • [ ] Warn for use of nameof within raw SQL interpolated strings. This typically happens when people mistakenly want to interpolate column names, but they get parameterized and the query fails.

Scenarios requiring identifying EF Core LINQ queries (could use common infrastructure):

  • [ ] #26856
  • [ ] #21663
  • [ ] https://github.com/dotnet/efcore/issues/28287#issuecomment-1162208130
  • [ ] #28449

One idea here would be to have users tell us in .editorconfig that all queryable LINQ queries are e.g. SQL Server; this would be an opt-in that could unlock provider-specific warnings/code fixes.

See #20206 for previous unanalyzed scenarios.

roji avatar Aug 16 '20 10:08 roji

composite-issue label? Or is that already implied by User Story?

vslee avatar Jan 03 '23 00:01 vslee

One area I've been thinking we could improve using analyzers is cleaning up people's OnModelCreating code. We've had a few workarounds that later get a better API. I suspect the old workarounds are just sitting around providing bad examples to people.

For example, the old way to configure precision and scale was using the column type:

// EF Core 1-4
modelBuilder.Entity<Entity>()
    .Property(e => e.DecimalProperty)
        .HasColumnType("decimal(18, 2)");

But in EF Core 5, we added a better way to do it:

// EF Core 5
modelBuilder.Entity<Entity>()
    .Property(e => e.DecimalProperty)
        .HasPrecision(18, 2);

In addition, I think there's an opportunity to discourage people from dropping down to the core metadata APIs. For example, we could also provide a fix for code like this:

var decimalProperty = modelBuilder.Entity<Entity>().Property(e => e.DecimalProperty).Metadata;
decimalProperty.SetPrecision(18);
decimalProperty.SetScale(2);

bricelam avatar May 19 '23 17:05 bricelam

IIUC now suppressions work as intended even when TreatWarningsAsErrors is set https://github.com/dotnet/efcore/issues/29582 and efcore contains some infrastructure to identify EF Core LINQ queries (maybe not in all cases, but at least in many relevant cases that are being used to pre-compile queries). Is this a good time to try and tackle some of these issues? (given that now there seems to be the infrastructure already in place, would some of these be good first issue candidates?)

ranma42 avatar May 05 '24 07:05 ranma42

@ranma42 the infrastructure introduced for identifying EF LINQ queries (for NativeAOT/precompiled queries) is very new, and is currently not being used in a Roslyn analyzer/source generator at all (but rather in a custom step, meant to be executed outside Roslyn at publish-time). So in theory you're absolutely right, and this does open up a future where we can start emitting/suppressing diagnostics inside LINQ queries; but in practice, it will take a while before this is something that we're able to tackle.

In any case, I definitely wouldn't consider any analyzer feature as a good first issue; writing analyzers/suppressors correctly (and efficiently!) can be quite tricky/advanced.

roji avatar May 06 '24 21:05 roji