bug#43559: 28.0.50; [PATCH] Add csharp support to cc-mode

[Theodor Thornhill]

Jostein Kjønigsen <jostein@secure.kjonigsen.net> writes:

> On 22.09.2020 15:10, Theodor Thornhill wrote:
>> Jostein Kjønigsen <jostein@secure.kjonigsen.net> writes:

[...]

> I still haven't looked at the code in your patch, but I've applied it to 
> latest Emacs Git master, and given it a test-run.
> When going over existing code, everything looks and feels nice. And it's 
> much faster than my "old", non-transferable code. So I definitely think 
> this is a /very good/ start.

Nice!

>
> While I hold no "position" in GNU or Emacs as a whole, I would consider 
> myself a C# veteran (since 2004), and as the old maintainer of old 
> csharp-mode, I think I have some experience which may be useful.
Most definitively.  Thank you for taking your time!

>
> So for the time being, I'll just promote myself to being the "critical 
> Emacs C#-user", and provide some feedback based on my expectations for 
> how editing C# should feel or behave.
>
> Please don't consider the following comments as negative towards your 
> work, or me in any way trying to dissuade you from getting this 
> mainlined. I'm just here to try to help you ensure that what you land is 
> going to be "good enough" so that it will be easier to argue for 
> inclusion in core-Emacs later on :)
None of your comment are taken as negative.  I'd much rather you'd be
critical early, so that we can get something nice quicker!

>
> So with all that said, so far I've noticed a few things which I think 
> arguably goes against C# convention, and would be nice to get fixed, if 
> possible. There's also a some things where I just feel it would "better" 
> to do things differently.
>

Before I answer your comments, I'll just let you know I'm still wrapping
my head around the whole cc-mode.  I might find better solutions to all
this later on.

>
> *First off: **Attributes are not handled properly. **
> *
>
> In java-mode annotations gets fontified and indented properly:
>
> In your current csharp-mode draft, we get no fontification, and we also 
> get a trailing indentation bug for the equivalent C# code:

Yeah, this is a difficult one, because the java version of this
implements lots of logic in the cc-engine.el, and for now, we don't.
However, my most recent patch seems to deal with this properly, as far
as I can tell.  I've tested with your example code and also with random
files in the roslyn repository.  Fontification is still sparse here.

>
> *Second: Object initializers are not indented properly.*
>
> Consider the following fairly simple case. Using the provided patch we 
> get the following indentation:

This one should be fixed in the attached patch.

> This one is arguably very hard to get right, because it's a conceptually 
> infinitely recursive problem. (You may initialize a property with 
> another object-initializer.)

I think the recursion case works as well.  I've made an attempt in the
attached 'test.cs'

>
> While solving this perfectly for all cases is clearly out the window, do 
> you think it would be possible at least to make 1-level 
> object-initilizers work?

Hopefully, we have it now!

>
> One the bright side /collection-/initializers seems to work just fine :)
>

Great!

>
> *Third: Lamda-function indentation when used with higher order functions
> *
>
> Lambdas also suffer from some unexpected indentation issues:

This one should be fixed in the latest patch.

>
> *Fourth: variable-fontification*
>
> Here I have no absolute C# convention to quote for absolute correctness, 
> but it kinda "feels wrong" to me at places.

> So we'll have to make due with imperfections, make some pragmatical 
> decisions on what we think will be good default/fallback values, and 
> that's OK.
>

Agreed

> Right now though, all implicitly typed variables (vars), local variables 
> with method-access and local fields with property-access are shown using 
> /font-lock-constant-face/ and that seems a bit off:

This case is fixed now.  It was due to the 'var' keyword was put in the
wrong basket.

> As for "_field" and "foo.", I'm not sure what the best fallback would 
> be. Without a language-engine to guide us, this is genuinely hard stuff 
> to get right.
>

Yeah, this one is kind of hard, so I've been ignoring it for a little
while.  The easiest thing should be to remove the highlighting, but not
sure if that is the best move so far.

>
> I'm sure just/discussing/ the///soft-rules/ for these aspects could take 
> months alone, not speaking for the effort required to actually making it 
> happen in code.
>
> Consider it something to aim for, but I wouldn't expect anyone to get 
> this right in a version 1 of anything.
>

Yeah, more cases are coming up all the time.  My initial focus has been
mostly on the indentation part, since font-locking is a 'bonus-feature'
in some sense.  However, they seem intimately connected in cc-mode.

>
> Otherwise, great work, and thank you for putting in this effort!

My pleasure :)


One issue I have now is that there are many rules in the c-style-alist,
and I think it would be better to design or reuse some constructs from
cc-engine to apply some other syntax rules to the code.  One example and
consequence of this is the fix of the attributes from your code.  It
aligns nicely, but destroys the indentation of the 'aligned ternary' (on
the bottom of the file).


Please, if you have time, take a look at the second patch, as well as
the test file.  Maybe you get some ideas, but I think some of the low
hanging fruits are found, and it is time to dive into the engine itself.

I really appreciate you taking your time to find these bugs for me.
Thanks.

--
Theo


P.S
Pasting the contents of test.cs here, since I apparently suck at email

/// Working

[Test]
public class Foo
{
    [Zoo]
    [Goo]
    public void Bar()
    {
        
    }
};

var x = new SomeType[]
{
    Prop1 = "foo",
    Prop2 = new SomeType2[]
    {
        Prop3 = true ? "this" : "that";
    }
    //
};


var x = new SomeType[] {
    new SomeType(a, b),
    new SomeType(b, c)
    // lol
};

/// Kind of working

goo.Select(i => {
    return goo.Select(i)
    .Foo()
    .Bar()
});

Foo foo = new Foo();
var bar1 = foo.Bar(_field);
Bar bar3 = foo.Bar(_field.Prop);

/// Not working

var x = foo
? bar
: baz;

v2-0001-Add-csharp-support-to-cc-mode.patch
Description: Text Data