search

dotnet / roslyn-analyzers

MIT License
1.58k stars 463 forks source link

RS0030 Banned type symbol not reported when property/method return type or parameter #2747

Open wachulski opened 5 years ago

wachulski commented 5 years ago

Analyzer package

Example: Microsoft.CodeAnalysis.BannedApiAnalyzers

Package Version

Example: v2.9.4 (Latest)

Diagnostic ID

RS0030

Repro steps

BannedSymbols.txt: 

T:N.B

.cs:

namespace N
{
    public class B { }

    public class C
    {
        B B => new B();
    }

    public interface I
    {
        B GetB(B otherB);
    }
}

Expected behavior

All four occurrences of B should be reported.

Actual behavior

Only C.B's symbol in expression body is reported (in new B();)

Evangelink commented 4 years ago

@genlu, @drewnoakes I had a look at the actual implementation and the code relies on operations to find banned usages. It doesn't seem that we can use operation to catch to the other cases reported in this issue. I know that Symbol actions are expensive but except for this solution and syntax based one (which then will rely on getting the symbol), I can't think of any other way to do so. Any idea?

mavasani commented 4 years ago

I am not sure I understand the value add for such a change. For example, if the return type is a banned type, then the analyzer will currently flag the return value. This is sufficient to fix the code for user. I believe we should close this issue as by design.

sharwell commented 4 years ago

Banned is banned. I agree with the the original expected behavior: 4 diagnostics should be reported.

sharwell commented 4 years ago

I know that Symbol actions are expensive

Symbol actions are not expensive. The expensive actions are syntax actions that use symbols via GetSymbolInfo.

mavasani commented 4 years ago

Banned is banned. I agree with the the original expected behavior: 4 diagnostics should be reported

I still feel there is absolute no value add in doing this work. Current approach does not find banned type usages in signatures, but it does so in all executable code. I claim this is sufficient for the user to make their code clean.

Evangelink commented 4 years ago

@sharwell Oh! Thanks for that clarification!

@mavasani It might be somehow a corner case but it could happen that you are defining an interface or abstract class in some common library that you are providing to your internal devs and yet your lead dev wants to make sure you won't use some type. With the current approach the "detection" will only occur after the code is delivered and at this point that's a bit of a nightmare to do some change. At the same time, I do share your point when you say that the current implementation will cover most of the cases and so the update isn't really top priority.

sharwell commented 4 years ago

I claim this is sufficient for the user to make their code clean.

We are using this analyzer to ensure IVT isolation in Roslyn will prevent partner code breaks. Failure to detect these situations would cause a partner code break for the described scenario, even if the interface is not implemented or used anywhere. Since we already must fix this for correct behavior of restricted IVTs, it doesn't seem problematic to apply it to RS0030 in addition.

Evangelink commented 4 years ago

@sharwell, @mavasani How do you want to proceed with this? Close? Implement? Something else?

mavasani commented 4 years ago

I would like to wait on this issue until someone can provide me a real user case of when existing RS0030 violations in code were not sufficient to catch these additional usages. Until then, it just feels like an unnecessary code churn. We seem to have sufficient feedback. I'd be fine if someone wants to submit a PR here.

OsirisTerje commented 3 years ago

@mavasani I believe it may hit when the usage differs, e.g. if I want to ban ICloneable, with T:System.ICloneable, it will not find any.

kaylumah commented 2 years ago

So if understand correctly this is by design?

public interface IExample
{
    // Does not trigger
    void Test(DateTime input);

    // Does not trigger
    DateTime DoSomething();

    void Other();
}

public class Example : IExample
{
    // arg does not trigger
    public void Test(DateTime input)
    {
        // does not trigger
        var x = input;

        // triggers here
        x.AddDays(1);
    }

    // does not trigger
    public DateTime DoSomething()
    {
        // triggers
        return new DateTime(2022,9, 17);
    }

    public void Other()
    {
        // triggers
        var x = new DateTime(2022, 1, 1);

        // triggers
        var y = DateTime.Now;
    }
}

So the official statement is that any usage of the type will trigger it?

That would mean the current approach will work for any scenario except .Interface / Abstract classes in a different dll than the implementation?

The config being used is, but perhaps I need additional lines?

T:System.DateTime;Always use System.DateTimeOffset over System.DateTime

Bouke commented 1 year ago

I would like to wait on this issue until someone can provide me a real user case of when existing RS0030 violations in code were not sufficient to catch these additional usages. Until then, it just feels like an unnecessary code churn.

When separating abstractions from implementations, one creates a project with (mostly) only interfaces. I cannot ban the use of certain APIs in this project, as the project doesn't "interact" with the banned APIs, but merely points to them in method signatures / return types. We have a WCF project like this where we define the contracts in a separate project. We don't want to expose any internal types through this project, so being able to ban types in this project would achieve that. However as it currently stands, no error is reported for this project.

TanukiSharp commented 11 months ago

@mavasani To echo what other people said here, a concrete use case we faced yesterday is, we want to ban DateTime in the benefit of DateTimeOffset for entity objects we use with EF Core. The problem is that it is perfectly fine for someone to create an assembly with types that have DateTime-type properties, but nobody will be able to consume it, so allowing declaration of banned types doesn't make sense.

In a context where the producer and the consumer are in the place, it's fine, but once they are lose-coupled, it just becomes a non-sense to ban only on one side.

mavasani commented 11 months ago

Given we have sufficient feedback here, I am fine if someone wants to submit a PR with the fix for this one.

TanukiSharp commented 11 months ago

I'm fine to give it a try.

TanukiSharp commented 11 months ago

@mavasani

I ran build.cmd and it worked fine, however running test.cmd failed with the following message:

Toolset version 8.0.0-beta.23463.1 has not been restored.

Here are the SDKs I have on my machine:

2.1.302
2.2.104
3.1.301
6.0.201
6.0.416
7.0.402
8.0.100-rc.2.23502.2

Do you have any idea how to fix that ? Thank you.

NB: Here is my dotnet --info:

dotnet --info
.NET SDK:
 Version:   8.0.100-rc.2.23502.2
 Commit:    0abacfc2b6

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22621
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\8.0.100-rc.2.23502.2\

.NET workloads installed:
 [wasm-tools-net6]
   Installation Source: VS 17.8.34205.153, VS 17.7.34202.233
   Manifest Version:    8.0.0-rc.2.23479.6/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.workload.mono.toolchain.net6\8.0.0-rc.2.23479.6\WorkloadManifest.json
   Install Type:              Msi

 [wasm-tools-net7]
   Installation Source: VS 17.8.34205.153
   Manifest Version:    8.0.0-rc.2.23479.6/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.workload.mono.toolchain.net7\8.0.0-rc.2.23479.6\WorkloadManifest.json
   Install Type:              Msi

 [maui-windows]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    8.0.0-rc.2.9373/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.maui\8.0.0-rc.2.9373\WorkloadManifest.json
   Install Type:              Msi

 [wasm-tools]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    8.0.0-rc.2.23479.6/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.workload.mono.toolchain.current\8.0.0-rc.2.23479.6\WorkloadManifest.json
   Install Type:              Msi

 [maui-maccatalyst]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    8.0.0-rc.2.9373/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.maui\8.0.0-rc.2.9373\WorkloadManifest.json
   Install Type:              Msi

 [android]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    34.0.0-rc.2.468/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.android\34.0.0-rc.2.468\WorkloadManifest.json
   Install Type:              Msi

 [maccatalyst]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    16.4.8968-net8-rc2/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.maccatalyst\16.4.8968-net8-rc2\WorkloadManifest.json
   Install Type:              Msi

 [maui-ios]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    8.0.0-rc.2.9373/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.maui\8.0.0-rc.2.9373\WorkloadManifest.json
   Install Type:              Msi

 [ios]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    16.4.8968-net8-rc2/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.ios\16.4.8968-net8-rc2\WorkloadManifest.json
   Install Type:              Msi

 [maui-android]
   Installation Source: VS 17.7.34202.233
   Manifest Version:    8.0.0-rc.2.9373/8.0.100-rc.2
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100-rc.2\microsoft.net.sdk.maui\8.0.0-rc.2.9373\WorkloadManifest.json
   Install Type:              Msi

Host:
  Version:      8.0.0-rc.2.23479.6
  Architecture: x64
  Commit:       0b25e38ad3

.NET SDKs installed:
  2.1.302 [C:\Program Files\dotnet\sdk]
  2.2.104 [C:\Program Files\dotnet\sdk]
  3.1.301 [C:\Program Files\dotnet\sdk]
  6.0.201 [C:\Program Files\dotnet\sdk]
  6.0.416 [C:\Program Files\dotnet\sdk]
  7.0.402 [C:\Program Files\dotnet\sdk]
  8.0.100-rc.2.23502.2 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.24 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.0-rc.2.23480.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.24 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.0-rc.2.23479.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.23 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.24 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.0-rc.2.23479.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
  x86   [C:\Program Files (x86)\dotnet]
    registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables:
  Not set

global.json file:
  C:\data\xxx\thirdparty\roslyn-analyzers\global.json

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download
Bouke commented 11 months ago

When I was working on this project, I adjusted some of these files to not reference/require vNext (in this case: 8). Not sure about the toolset, I think I also couldn't find out how to install those.

Requiring these dependencies is not great for external contributors. I would applaud a mode/configuration that allows me to contribute using just the latest GA sdk. CI would do verification testing against vNext.

TanukiSharp commented 11 months ago

@mavasani

Any help to get test.cmd to run and work please ? Without this, it's impossible to validate if changes made are correct or breaking.

Thanks.

mavasani commented 11 months ago

@TanukiSharp I believe there are changes to global.json in https://github.com/dotnet/roslyn-analyzers/pull/6972/files that should likely address your issue once that PR is merged. Can you try restore after locally making the global.json changes from that PR?

TanukiSharp commented 11 months ago

@mavasani Thank you for your answer. The problem remains the same, some versions used are not yet available to mere mortals like me.

> dotnet restore
The command could not be loaded, possibly because:
  * You intended to execute a .NET application:
      The application 'restore' does not exist.
  * You intended to execute a .NET SDK command:
      A compatible .NET SDK was not found.

Requested SDK version: 8.0.100-rtm.23506.1
global.json file: xxx\thirdparty\roslyn-analyzers\global.json

Installed SDKs:
2.1.302 [C:\Program Files\dotnet\sdk]
2.2.104 [C:\Program Files\dotnet\sdk]
3.1.301 [C:\Program Files\dotnet\sdk]
6.0.201 [C:\Program Files\dotnet\sdk]
6.0.416 [C:\Program Files\dotnet\sdk]
7.0.402 [C:\Program Files\dotnet\sdk]
8.0.100-rc.2.23502.2 [C:\Program Files\dotnet\sdk]

Install the [8.0.100-rtm.23506.1] .NET SDK or update [xxx\thirdparty\roslyn-analyzers\global.json] to match an installed SDK.

Learn about SDK resolution:
https://aka.ms/dotnet/sdk-not-found

I've searched for *rtm* in the dotnet repo but apparently there are no binaries available yet.

I tried to tweak the versions in global.json with other RCs, but it always fail one way or another.

mavasani commented 11 months ago

Requested SDK version: 8.0.100-rtm.23506.1 global.json file: xxx\thirdparty\roslyn-analyzers\global.json

@TanukiSharp Do you still get the issue with the below contents of global.json? Nothing should be pulling onto 8.0.100-rtm.23506.1

{
  "tools": {
    "dotnet": "8.0.100-rc.2.23502.2",
    "runtimes": {
      "dotnet": [
        "3.1.7",
        "6.0.11"
      ]
    },
    "vs": {
      "version": "17.7.0"
    },
    "xcopy-msbuild": "17.8.1-2"
  },
  "sdk": {
    "version": "8.0.100-rc.2.23502.2",
    "allowPrerelease": true,
    "rollForward": "patch"
  },
  "msbuild-sdks": {
    "Microsoft.DotNet.Arcade.Sdk": "9.0.0-beta.23528.2"
  }
}
TanukiSharp commented 11 months ago

@mavasani Thank you for the very quick answer. With the above mentioned global.json I get the following error on .\test.cmd:

> .\test.cmd
Toolset version 9.0.0-beta.23528.2 has not been restored.

I just updated all my installs of VS 2022 and Preview, just in case, but it didn't help.

mavasani commented 11 months ago

@mavasani Thank you for the very quick answer. With the above mentioned global.json I get the following error on .\test.cmd:

> .\test.cmd
Toolset version 9.0.0-beta.23528.2 has not been restored.

I just updated all my installs of VS 2022 and Preview, just in case, but it didn't help.

@Evangelink @carlossanlop any suggestions here?

carlossanlop commented 11 months ago

I just merged https://github.com/dotnet/roslyn-analyzers/pull/6972 into main, which downgrades the rtm version used in global.json to the last publicily available one, rc2.

TanukiSharp commented 10 months ago

The command .\Test.cmd still fails with the following:

> .\Test.cmd
Toolset version 8.0.0-beta.23463.1 has not been restored.

The global.json is still on "Microsoft.DotNet.Arcade.Sdk": "8.0.0-beta.23463.1" but even if I set it to 9.0.0-beta.23528.2, it fails with:

Toolset version 9.0.0-beta.23528.2 has not been restored.

I've run .\Restore.cmd and .\Build.cmd and both commands work fine with both versions of Microsoft.DotNet.Arcade.Sdk, but the .\Test.cmd command keeps failing.

TanukiSharp commented 10 months ago

@carlossanlop Any update ?

mavasani commented 10 months ago

@TanukiSharp we will look at the Test.cmd issue and share an update. However, you can workaround for now by running the Banned API Analyzers unit tests in VS from the TestExplorer. That should be sufficient for the changes required for this Issue

sgryphon commented 9 months ago

Hi, just ran into this scenario, where there was a class with a DateTime property that is not picked up by the analyser.

Initially it was being set by x.CreateDate = DateTimeOffset.Now.DateTime, which I changed to x.CreateDate = timeProvider.GetLocalNow().DateTime, as I have another rule banning Now/UtcNow.

After that the error went away, as the .DateTime conversion is not in my code, and the property type is not being analysed. The record is written to a DB as JSON, so auto-serialised, and we don't have code that reads the property.

My expectation would be that the analyser picked up this usage of the banned type.

(And yes, I know the property name is also bad, as it is not a date; it should be x.CreateTimestamp or similar)

baterja commented 3 months ago

I encountered that issue in a POC of BannedApiAnalyzers when trying to add a 2nd example (the 1st is for DateTime.Now) to forbid usage of IActionResult as a controller's method return type to encourage Results<> usage. It's 1 of my 3 ideas (the 3rd being Newtosoft.Json) so I just lost a serious percentage of "POC ammo" 😢