dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.95k stars 4.02k forks source link

'Convert to raw string without leading whitespace' erroneously strips trailing whitespace #62376

Open jnm2 opened 2 years ago

jnm2 commented 2 years ago

Version Used: 17.3.0-p2.0

This is not the semantics change described by the light bulb fix.

var whereClause = "Age >= 18";

var x = @"
    select FirstName, LastName
    from dbo.People
    where " + whereClause;

💡 "Convert to raw string without leading whitespace (may change semantics)":

var whereClause = "Age >= 18";

var x = """
    select FirstName, LastName
    from dbo.People
    where
    """ + whereClause;

The resulting string contains whereAge >= 18 instead of where Age >= 18.

AraHaan commented 2 years ago

I think this behavior is what the feature was intended to do. I would say that whereClause should prefix itself with the space that is lost (or have it check for the space prefix and if not there insert it to be first).

CyrusNajmabadi commented 2 years ago

I did write this:

            // Remove all trailing whitespace and newlines from the final string.
            while (result.Count > 0 && (IsCSharpNewLine(result[^1]) || IsCSharpWhitespace(result[^1])))
                result.RemoveAt(result.Count - 1);

But i can't remember why :). I'm not opposed to removing this.

davidwengier commented 2 years ago

Perhaps that bit of code should only apply if the string doesn't have anything after it except a semi-colon? I quite like that:

var x = @"
class C
{
}
";

is converted to

var x = """
class C
{
}
""";

In fact I like the removal of trailing whitespace so much I logged https://github.com/dotnet/roslyn/issues/61267 (just in case you felt like fixing that at the same time 😛)

CyrusNajmabadi commented 2 years ago

Ah yes. That was the intent. It probably makes sense to delete whitespace and newlines up through and trailing whitespace on the last content line

jnm2 commented 2 years ago

I would say that whereClause should prefix itself with the space that is lost (or have it check for the space prefix and if not there insert it to be first).

@AraHaan This seems like an onerous amount of overhead for something that's not really dynamic, just factored across multiple statements. Either way, it's extremely hard to notice that the trailing whitespace has changed; it's not a good pit. Leading whitespace is specifically called out in the light bulb in a way that sounds exclusive.

jnm2 commented 2 years ago

@davidwengier @CyrusNajmabadi I don't think I like that conversion, and I really don't like the fact that we mention leading whitespace in a way that sounds like nothing else will be changed.

If we have to do any trailing trimming, could we please make this simply remove trailing \r\n or \n, once?

var x = @"
class C
{
}

";

Should not be converted to:

var x = """
class C
{
}
""";
CyrusNajmabadi commented 2 years ago

@jnm2 definitely. Feel free to create PR here :)