Page MenuHomePhabricator

[clang-format] successive C# attributes cause line breaking issues
ClosedPublic

Authored by MyDeveloperDay on May 28 2021, 3:34 AM.

Details

Summary

D74265: [clang-format] Improve handling of C# attributes reduced the aggressiveness of line breaking following C# attributes, however this change removed any support for attributes on properties, causing significant ugliness to be introduced.

This revision goes some way to addressing that by re-introducing the more aggressive check to mustBreakBefore(), but constraining it to the most common cases where we use properties which should not impact the "caller info attributes" or the "[In , Out]" decorations that are normally put on pinvoke

It does not address my additional concerns of the original change regarding multiple C# attributes, as these are somewhat incorrectly handled by virtue of the fact its not recognising the second attribute as an attribute at all. But instead thinking its an array.

The purpose of this revision is to get back to where we were for the most common of cases as a stepping stone to resolving this. However D74265: [clang-format] Improve handling of C# attributes has broken a lot of C# code and this revision will go someway alone to addressing the majority.

Diff Detail

Event Timeline

jbcoe accepted this revision.May 28 2021, 3:49 AM

Thanks for this!

This revision is now accepted and ready to land.May 28 2021, 3:49 AM

Thanks for this!

I tried to handle the multiple attributes using the "parsing technique" you used, instead of the "mustBreaking technique" but every time I do this I end up putting a newline after every array!

Add additional unit test, and an additional check to ensure multiple attributes are broken, while @jbcoe this looks very similar to the original rule, the original rule actually had an || in the expression and not an &&, this I believe caused and issue where the adjacent [] [] caused an issue because we were breaking on an attribute and and array. By virtue of the fact the attributes are not correctly identified. Which concerns me a little,

void myFunction([In][Out] int a)

M=0 C=1 T=AttributeSquare S=0 F=0 B=0 BK=0 P=140 Name=l_square L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=identifier L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x25b3990 Text='In'
M=0 C=0 T=AttributeSquare S=0 F=0 B=0 BK=0 P=63 Name=r_square L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
M=0 C=0 T=*ArraySubscriptLSquare* S=0 F=0 B=0 BK=0 P=240 Name=l_square L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=259 Name=identifier L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x25b39b0 Text='Out'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=243 Name=r_square L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'

but this subsequent change seems to handle the cases where 2 attributes are decorating a property (for the most part)

M=0 C=0 T=AttributeSquare S=0 F=0 B=0 BK=0 P=43 Name=r_square L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
M=1 C=1 T=AttributeSquare S=1 F=0 B=0 BK=0 P=220 Name=l_square L=131 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['

My view is that [In][Out] should/could possibly be written as [In , Out] to avoid the error that at least your review seemed to want to be fixing.

And I think that using attributes on properties is perhaps more likely to be pervasive though out C# code than having them on pinvoke.

This update, adds a few more tests to try and uncover attitional places I would have expect this to break, (it doesn't) but I want to have more protection from the tests

From my tests I see one place where I see an issue, And seemingly for some reason this completely messes up even identifying that the attributes are themselves attributes. This is definitely a common problem. But the addition of either public/private/internal/protected immediately fixes that and I personally consider that a C# best practice anyway.

[JsonProperty("gender")] string gender { get; set; }


 M=0 C=0 T=ArraySubscriptLSquare S=1 F=0 B=0 BK=0 P=0 Name=l_square L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=239 Name=identifier L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x25b37f0 Text='JsonProperty'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=223 Name=l_paren L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=259 Name=string_literal L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='"gender"'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=243 Name=r_paren L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=223 Name=r_square L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=identifier L=31 PPK=2 FakeLParens= FakeRParens=0 II=0x25b3418 Text='string'
 M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=38 PPK=2 FakeLParens= FakeRParens=0 II=0x25b3830 Text='gender'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=l_brace L=40 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=59 Name=identifier L=44 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x25b2df0 Text='get'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=semi L=45 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=40 Name=identifier L=49 PPK=2 FakeLParens= FakeRParens=0 II=0x25b2eb8 Text='set'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=semi L=50 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=';'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=40 Name=r_brace L=52 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='}'

If we are in agreement that this doesn't revert your intended changes, I'd like to commit this change as an interim step. I'd really like at least these aspect resolved before the 13 branch out.

curdeius accepted this revision.May 28 2021, 1:07 PM

LGTM.

clang/lib/Format/TokenAnnotator.cpp
3529

Nit.