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

Combining #if SYNC_ONLY with if(...) or using(...) ends up with missing code #45

Open
persn opened this issue Jan 16, 2024 · 4 comments

Comments

@persn
Copy link

persn commented Jan 16, 2024

I don't know how well supported #if SYNC_ONLY #else "endif is supposed to be, but if I try doing the following it works perfectly like expected

[Zomp.SyncMethodGenerator.CreateSyncVersion]
public static async Task Foo2Async()
{
#if SYNC_ONLY
    _ = true;
#else
    _ = true;
#endif
    await Task.Delay(100).ConfigureAwait(false);
}

However if I do something that has () like if or using inside the directives it'll lose all code after #endif

[Zomp.SyncMethodGenerator.CreateSyncVersion]
public static async Task FooAsync()
{
#if SYNC_ONLY
    if (true)
#else
    if (true)
#endif
        await Task.Delay(100).ConfigureAwait(false);
}

Because the generated code ends up like this

public static void Foo()
{
    if (true)
}
@lsoft
Copy link

lsoft commented Jan 16, 2024

@virzak I found that ISG does not like #else at any symbol:

        [Zomp.SyncMethodGenerator.CreateSyncVersion]
        public async Task<T> FooAsync<T>()
        {
#if MY_SPECIAL_SYMBOL
            if (true)
#else
            if (true)
#endif
            { }

            return default;
        }

gives me

        public T Foo<T>()
        {

            return default;
        }

It would be more correct if ISG converts these non-related symbols as is withot touching it:

        public T Foo<T>()
        {
#if MY_SPECIAL_SYMBOL
            if (true)
#else
            if (true)
#endif
            { }

            return default;
        }

I suspect this may become a very difficult task, for example this will fail:

#define MY_SPECIAL_SYMBOL

...

        [Zomp.SyncMethodGenerator.CreateSyncVersion]
        public async Task<T> FooAsync<T>()
        {
#if MY_SPECIAL_SYMBOL
            await Task.Delay(1);
#else
            await Task.Delay(1);
#endif
            return default;
        }

ISG should take a look inside of all branches to convert the code, now it looks only to the active branch, leave the other as is (with async style inside the sync method).

@virzak
Copy link
Contributor

virzak commented Mar 11, 2024

Yes, this is more complex than it seems. I can definitely look into this, however I'm wondering a workaround is feasible?

A statement like this:

#if SYNC_ONLY
    if (true)
#else
    if (true)
#endif
        await Task.Delay(100).ConfigureAwait(false);

would require the generator to construct if statement from disabled trivia.

Would this work for now?

#if SYNC_ONLY
        if (true)
            global::System.Threading.Thread.Sleep(100);
        
#else
        if (true)
            await Task.Delay(100).ConfigureAwait(false);
#endif

@persn
Copy link
Author

persn commented Mar 18, 2024

That probably looks reasonable for a naive one-liner, but in production code there's usually many lines within curly brackets which the code generator handles equally poorly. I should mention that the same problem also happens for usings, so then I would need to copy-paste the same code twice.

Right now it looks like to me that for the methods where this is a problem it's better to not use the code generator at all and just keep and maintain a sync and a async manually.

@GerardSmit
Copy link
Contributor

As workaround: it's also possible to define a constant value in the method like this:

#if SYNC_ONLY
    const bool isAsync = false;
#else
    const bool isAsync = true;
#endif

When C# compiles the if-statements:

if (isAsync) await Task.Delay(100).ConfigureAwait(false);

it sees that isAsync is a constant value, and removed the if-statement in the release build.
See the SharpLab here, you can see that "Async" is not in the decompiled C#-code on the right.

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

No branches or pull requests

4 participants