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

Code in IfDirective can't currently be formatted #15

Closed
belav opened this issue Feb 27, 2021 · 8 comments · Fixed by #405
Closed

Code in IfDirective can't currently be formatted #15

belav opened this issue Feb 27, 2021 · 8 comments · Fixed by #405
Assignees
Milestone

Comments

@belav
Copy link
Owner

belav commented Feb 27, 2021

Given the following IfDirective, roslyn does not parse the contents because DEBUG is not defined

#if DEBUG
        Console.WriteLine("Rosyln does not parse this because DEBUG is not defined");
#endif

The contents are just a block of text.
Can I determine some way to parse those contents and format them? Currently they are just written out as is.

@belav
Copy link
Owner Author

belav commented Jul 24, 2021

PreprocessorSymbols can be passed like so

            var syntaxTree = CSharpSyntaxTree.ParseText(
                code,
                new CSharpParseOptions(
                    LanguageVersion.CSharp9,
                    DocumentationMode.Diagnose,
                    preprocessorSymbols: new[] { "DEBUG" }
                ),
                cancellationToken: cancellationToken
            );

We also have cases where we can't just send in one symbol. In this case we'd need to parse with and without DEBUG


public class ClassName {
#if DEBUG
    public string ShortPropertyName {
        get;
        set; 
    }
#else 
    public string ShortPropertyName {
        get;
        set; 
    }
#endif
}

The problems are

  1. How do we know what preprocessor symbols to pass?
  2. How do we combine the results of multiple formats, when we have a situation with an #if #else?

The idea starting to take shape in my head

  1. On the first pass through of formatting - keep track of the different symbols needed. In the example above we know we need to format without a symbol (already happened) and with DEBUG
  2. During that first pass, add something to the DocTree that can be replaced later. Or a pointer to a location in the DocTree.
  3. Parse the file again, using DEBUG. Then run the formatter again (or somehow only run the formatter on the areas that are affected)
  4. Take the Doc that was produced from the DEBUG area, and insert it into the original DocTree
  5. Print the DocTree

This will probably be complicated quite a bit by the situation in #121

@svermeulen
Copy link

svermeulen commented Aug 15, 2021

What might be helpful as a workaround for now is for csharpier to support being passed the list of defines to use. This would unblock those of us like myself that have a lot of code inside preprocessor blocks that are currently skipped, since we could just run csharpier multiple times for all the different preprocessor flags we want covered

@belav
Copy link
Owner Author

belav commented Aug 15, 2021

I think your workaround won't be too difficult to implement... and actually csharpier may just be able to reformat the file itself multiple times. First without any defines and then once for each define it finds. That will be way easier than my original idea if it works.
I'll make sure some way to deal with defines makes it into the next release.

@belav belav modified the milestones: Beta, 0.9.9 Aug 15, 2021
@svermeulen
Copy link

What about complex conditionals like #if (FOO && BAR) || QUX? To cover those, you'd have to format for every possible combination of defines, which would probably not be practical

Either that or read which different combinations of flags are actually used. Does Roslyn expose that information?

@belav
Copy link
Owner Author

belav commented Aug 15, 2021

doh, you are right.
I did get it working for a basic if DEBUG and (FOO && BAR) || QUX is exposed as a SyntaxNode by roslyn, so theoretically the approach may work. But trying to determine all possible combinations may not be feasible.
Working for basic cases is at least a step in the right direction.

@belav
Copy link
Owner Author

belav commented Aug 16, 2021

My current plan

  1. Get the basic forms of if directives formatting automatically. This is basically done, but I want to see if supporting a few more cases is possible. These are the supported cases - https://github.com/belav/csharpier/blob/directives/Src/CSharpier.Tests/TestFiles/Directives/PreprocessorSymbols.cst
  2. Add the ability to supply sets of preprocessor symbols in the config file that will be used to format files. It would look something like
    preprocessorSymbols: [
        ["FOO", "BAR"],
        ["QUX"],
        ["DEBUG"]
    ]

Long term we can hopefully handle the more complicated conditions automatically without resorting to formatting a file with every possible combination. With just 6 symbols there would be 720 combinations.

Nested directives have me a little worried though. This is valid

#if DEBUG1
#if OTHER
    public     string ShortPropertyName;
#endif
#endif

Supplying the symbol sets per file is another idea I had (if we can't figure the sets out ourselves). It would be more efficient for large code bases where only a few files need them.

@svermeulen
Copy link

I wonder if the order of format operations for the different defines could affect the result? If so it might be important to make that order consistent

@belav belav mentioned this issue Aug 16, 2021
10 tasks
belav added a commit that referenced this issue Aug 17, 2021
belav added a commit that referenced this issue Aug 18, 2021
belav added a commit that referenced this issue Aug 20, 2021
Adding some basic validation support for disabled text
closes #15
belav added a commit that referenced this issue Aug 20, 2021
Adding some basic validation support for disabled text
closes #15
@belav
Copy link
Owner Author

belav commented Aug 20, 2021

I wonder if the order of format operations for the different defines could affect the result? If so it might be important to make that order consistent

I just tested this and you were right. Good call!

If I went with my original plan of combining doc trees, then I think that could be avoided. But I think that approach is just not viable. For example how would you possibly combine the doc tree with code like this?

#if LOOP

            do
            {
#endif
#if TRY
            try
            {
#endif
#if LOOP
                do
                {
                    for (int i = 0; i < 10; i++)
                    {
#endif

This is also a good example of a case where the order it is formatted in affects the indentation level of the code after the #endif

I think for now just keeping the order consistent like you suggested will avoid the problem.

belav added a commit that referenced this issue Aug 23, 2021
Adding some basic validation support for disabled text
closes #15
belav added a commit that referenced this issue Aug 23, 2021
* Support for basic if/elif directives

Adding some basic validation support for disabled text
closes #15

* Fixing unit tests

* Making some progress on edge cases

* Self code review

* Change format of PreprocessorSymbols

* Self code review

* Cleanup after rebase

* Cleaning up some edge cases around new lines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants