Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Possible fix for issue #51 #52

Closed
wants to merge 1 commit into from
Closed

Possible fix for issue #51 #52

wants to merge 1 commit into from

Conversation

danielcweber
Copy link

According to the Roslyn sources (here and here) the initial state of a state machine that Roslyn creates for an async method is -1. Code Contracts assumes that for Roslyn, the initial state is 0 and thus fails in rewriting async methods compiled by Roslyn. See #51 for more details.

…compiled by Roslyn start with an initial state == 0 seems to be false. I can only tell from decompiling such assemblies since I have no insight into Roslyn.
@danielcweber danielcweber mentioned this pull request May 19, 2015
@matteo-mosca
Copy link

I'm actually looking forward to this. This is the only thing preventing me to migrate my current work to .net 4.6 and aspnet v5.

@Daniel-Svensson
Copy link

👍 This solves my problems with async.and roslyn.

And it also seems to resolve #38

@SergeyTeplyakov
Copy link
Contributor

Few questions/suggestions:

  1. Please update a comment and put following link as a proof that Roslyn is using the same constants as old compiler: https://github.com/dotnet/roslyn/blob/b6484300dfafb43af0c27e542ec457a7583e1aa8/src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineStates.cs

  2. Is there any changes in tests? What the impact from end-user perspective?

  3. I see another place in the code with a similar logic:

Extractor.cs:GetContractClumpFromMoveNext

var initialState = moveNext.IsAsync && !isRoslyn ? -1 : 0;
moveNext.MoveNextStartState = initialState;
originalContractPosition = null;
int statementIndex;
Contract.Assume(moveNext.Body != null);
Contract.Assume(moveNext.Body.Statements != null);
int blockIndex = ContractStartInMoveNext(this.contractNodes, moveNext, out statementIndex, iteratorMethod, isRoslyn);

I was unable to find usages for MoveNextStartState but for the sake of consistency I would suggest to change this as well.

Thanks a lot for investing time in this project!

@SergeyTeplyakov
Copy link
Contributor

Closed as dup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants