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

Async breaks MustNotResolveCurrentTimeViaDateTimeConventionSpecification #62

Closed
gkinsman opened this issue Jul 29, 2019 · 5 comments

Comments

@gkinsman
Copy link

commented Jul 29, 2019

Hi there!

Given a type:

public class Foo
{
    public void Bar()
    {
        var thing = DateTime.Now;
    }
}

the following IL is generated:

Foo.Bar:
IL_0000:  nop         
IL_0001:  call        System.DateTime.get_Now
IL_0006:  stloc.0     // thing
IL_0007:  ret  

which is easily detected by the MustNotResolveCurrentTimeViaDateTimeConventionSpecification which searches for any get_Now calls on DateTime.

What happens if this method is made async though (ignoring the void return type)?

public class Foo
{
    public async void Bar()
    {
        var thing = DateTime.Now;
    }
}

Foo.Bar now looks like:

Foo.Bar:
IL_0000:  newobj      UserQuery+Foo+<Bar>d__0..ctor
IL_0005:  stloc.0     
IL_0006:  ldloc.0     
IL_0007:  ldarg.0     
IL_0008:  stfld       UserQuery+Foo+<Bar>d__0.<>4__this
IL_000D:  ldloc.0     
IL_000E:  call        System.Runtime.CompilerServices.AsyncVoidMethodBuilder.Create
IL_0013:  stfld       UserQuery+Foo+<Bar>d__0.<>t__builder
IL_0018:  ldloc.0     
IL_0019:  ldc.i4.m1   
IL_001A:  stfld       UserQuery+Foo+<Bar>d__0.<>1__state
IL_001F:  ldloc.0     
IL_0020:  ldfld       UserQuery+Foo+<Bar>d__0.<>t__builder
IL_0025:  stloc.1     
IL_0026:  ldloca.s    01 
IL_0028:  ldloca.s    00 
IL_002A:  call        System.Runtime.CompilerServices.AsyncVoidMethodBuilder.Start<<Bar>d__0>
IL_002F:  ret

with the call to DateTime.Now pushed to the state machine, here it's the first step:

d__0.MoveNext:
IL_0000:  ldarg.0     
IL_0001:  ldfld       UserQuery+Foo+d__0.<>1__state
IL_0006:  stloc.0     
IL_0007:  nop         
IL_0008:  ldarg.0     
IL_0009:  call        System.DateTime.get_Now
IL_000E:  stfld       UserQuery+Foo+d__0.5__1
IL_0013:  leave.s     IL_002D
IL_0015:  stloc.1     
IL_0016:  ldarg.0     
IL_0017:  ldc.i4.s    FE 
IL_0019:  stfld       UserQuery+Foo+d__0.<>1__state
IL_001E:  ldarg.0     
IL_001F:  ldflda      UserQuery+Foo+d__0.<>t__builder
IL_0024:  ldloc.1     
IL_0025:  call        System.Runtime.CompilerServices.AsyncVoidMethodBuilder.SetException
IL_002A:  nop         
IL_002B:  leave.s     IL_0041
IL_002D:  ldarg.0     
IL_002E:  ldc.i4.s    FE 
IL_0030:  stfld       UserQuery+Foo+d__0.<>1__state
IL_0035:  ldarg.0     
IL_0036:  ldflda      UserQuery+Foo+d__0.<>t__builder
IL_003B:  call        System.Runtime.CompilerServices.AsyncVoidMethodBuilder.SetResult
IL_0040:  nop         
IL_0041:  ret

This makes the convention pass when it shouldn't, as the call to the prohibited method is now in a separate type to that that the search is being performed on.

Is there some way to follow the generated state machine's type and search within the generated code? Maybe this is a convention better suited to roslyn, as it has the raw syntax tree?

@andrewabest

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2019

Hrmm good catch @gkinsman - I can only assume that this would apply for any property accessors within async methods? We can get the async state machine's type - check out https://github.com/andrewabest/Conventional/blob/master/Conventional/Conventions/Cecil/LibraryCodeShouldCallConfigureAwaitWhenAwaitingTasksConventionSpecification.cs#L82

Let me have a play with this today - hopefully it is easy enough to retrieve the state machine type for any state machines in our parent type and browse their code for offending useages.

@andrewabest

This comment has been minimized.

Copy link
Owner

commented Jul 30, 2019

That was fairly straight forward :) can you have a gander at the PR? If you think it has solved the problem too, I'll merge it in once you've given it the thumbs up and the build is green.

@gkinsman

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

Looks great! Have added some comments and a PR to your PR for some additional edge cases and a minor refactoring, let me know what you think!

@andrewabest

This comment has been minimized.

Copy link
Owner

commented Aug 7, 2019

Sorry about the break in comms George, will take a peek at this today!

@andrewabest

This comment has been minimized.

Copy link
Owner

commented Aug 7, 2019

Team work makes the dream work! Thanks for your help with this one @gkinsman.

@andrewabest andrewabest closed this Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.