Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-deterministic System.DateTime usage Roslyn Analyzer #284

Merged
merged 21 commits into from
Apr 18, 2024

Conversation

allantargino
Copy link
Member

@allantargino allantargino commented Apr 8, 2024

This analyzer produces the diagnostic DURABLE0001, is the equivalent of DF0101 in the in-proc version of the analyzer. It looks for usages of System.DateTime.(Now|UtcNow|Today) in orchestration methods (or their invocations).

It used a stateful analyzer (example) in a base class to gather information about existent orchestration methods and their invocations and, in parallel, the concrete implementation looks for the DateTime usages. Last, after the compilation has ended, it matches if the violations happen in orchestrations and reports them as diagnostics.

I expect to use this abstraction/pattern to look for other usages to implement next rules.

Besides, this PR used the Release Tracking Analyzers and included two additional files to help keep tracking of released diagnostics.

@allantargino
Copy link
Member Author

allantargino commented Apr 8, 2024

hm that's weird - the first commit build failed because it failed to compile and find xUnit references. I expected it to just use the common Directory.Build.targets.

Then, I manually added the package reference (same 2.4.1 version) and the build step worked, but now tests are failing as well 🙃

     1>Project "/home/runner/work/durabletask-dotnet/durabletask-dotnet/Microsoft.DurableTask.sln" (1) is building "/home/runner/work/durabletask-dotnet/durabletask-dotnet/test/Analyzers.Test/Analyzers.Test.csproj" (27) on node 4 (VSTest target(s)).
    27>_VSTestConsole:
         MSB4181: The "VSTestTask" task returned false but did not log an error.
    27>Done Building Project "/home/runner/work/durabletask-dotnet/durabletask-dotnet/test/Analyzers.Test/Analyzers.Test.csproj" (VSTest target(s)) -- FAILED.

My VS runs them all fine:
image

On the other hand, I was able to locally reproduce the issue using dotnet test. Investigating..

@allantargino
Copy link
Member Author

allantargino commented Apr 8, 2024

fixed :)
The reference to xUnit was not being imported because the test project had the suffix Test instead of Tests 😄 (this check + this variable were adding it).

If necessary, I can take the fix and merge it though another PR. Just please let me know 👍

Copy link
Member

@jviau jviau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this analyzer work for non-function triggers as well? IE:

public class MyOrchestrator : TaskOrchestrator<Input, Output>
{
    public async Task<Output> RunAsync(TaskOrchestrationContext context, Input input)
    {
        var time = DateTime.UtcNow; // will this be checked?
    }
}

src/Analyzers/Helpers/KnownTypeSymbols.cs Outdated Show resolved Hide resolved
src/Analyzers/Helpers/RoslynExtensions.cs Outdated Show resolved Hide resolved
@allantargino
Copy link
Member Author

As it is it doesn't support orchestrators implemented through ITaskOrchestrator. I am going to investigate this scenario a bit right and get back to you.

@jviau
Copy link
Member

jviau commented Apr 9, 2024

As it is it doesn't support orchestrators implemented through ITaskOrchestrator. I am going to investigate this scenario a bit right and get back to you.

That is fine if it doesn't support it right now - we can review this PR for just the DF scenario. Adding support for the contract in another PR will help keep the PRs small. Just wanted to make sure we were aware of that and tracking that work.

Copy link
Contributor

@bachuv bachuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this analyzer!

@allantargino
Copy link
Member Author

allantargino commented Apr 10, 2024

Hi @jviau and @bachuv, I just fixed the code based on the comments above. Thanks for the great suggestions. Now the analyzer supports Durable Functions, TaskOrchestrators and Func based orchestrations. Besides, I moved the strings to a resource file. I might need to increase code coverage based on the new orchestrations support, but as I experiment with other analyzers I will think how to reuse test code and address that too.

@allantargino allantargino requested review from bachuv and jviau April 10, 2024 14:06
Copy link
Member

@jviau jviau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not experienced in writing analyzers so can't comment much on the implementation details. I'll look at it further but probably won't hold up the PR on it (as this will be preview).

I think it may be valuable for us to formally detail how our analyzers will follow method call chains that originate in an orchestration. How will this behave with static, instance, interfaces, abstract, etc methods? And as we create this spec we should consider if this could impact non-orchestration scenarios.

What is the expected behavior of the below code:

public static class SomeHelperClass
{
    public void HelperMethod() { // uses DateTime.Now }
}

public static class Orchestrations
{
    public static void MyOrchestrator([OrchestrationTrigger] TaskOrchestrationContext context)
    {
        SomeHelperClass.HelperMethod();
    }
}

public static class Other
{
    public static void OtherMethod([SomeOtherTrigger] Object object)
    {
      SomeHelperClass.HelperMethod(); // we have a non orchestration use of this method also.
    }
}

src/Analyzers/Analyzers.csproj Outdated Show resolved Hide resolved
src/Analyzers/Orchestration/OrchestrationAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/Orchestration/OrchestrationAnalyzer.cs Outdated Show resolved Hide resolved
src/Analyzers/RoslynExtensions.cs Outdated Show resolved Hide resolved
src/Analyzers/Analyzers.csproj Outdated Show resolved Hide resolved
src/Analyzers/Analyzers.csproj Show resolved Hide resolved
the previous version wasn't recognizing the latest c# empty collection initializers linting rules
@bachuv bachuv merged commit a275358 into microsoft:main Apr 18, 2024
2 checks passed
@allantargino allantargino deleted the datetime-analyzer branch April 22, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants