Fix record short syntax rewriting issue
Closed this issue · 4 comments
Related to discussion in #111
I believe we could have a small patch that takes care of renaming records under the shortcut syntax with the missing _patched suffix.
Actually the issue is witch the record keyword itself not specifically on the shortcut syntax of records.
Also :
- Tried to add a script override, for instance adding suffix
public record LeapAction__Patched_
- FSR still rewrites it
public record LeapAction
- The constructor gets rewritten with the suffix though :
public LeapAction__Patched_()
. - We end up with the compiler throwing on the ctor
error CS1520: Method must have a return type'
as the ctor is recognised as a regular function.
Alright, I think I got it, I post the workaround here and either I'll do a PR or someone else after further testing, for now it looks okay with the regular and the shotcut syntax.
Add in DynamicCompilationBase just before user Scripts overrides :
root = new RecordeRewriter(DebugWriteRewriteReasonAsComment).Visit(root);
Rewriter :
using FastScriptReload.Runtime;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
namespace FastScriptReload.Editor.Compilation.CodeRewriting
{
class RecordeRewriter : FastScriptReloadCodeRewriterBase
{
public RecordeRewriter(bool writeRewriteReasonAsComment)
: base(writeRewriteReasonAsComment)
{
}
public override SyntaxNode VisitRecordDeclaration(RecordDeclarationSyntax node)
{
return AdjustRecordName(node, node.Identifier);
}
private SyntaxNode AdjustRecordName(RecordDeclarationSyntax node, SyntaxToken nodeIdentifier)
{
var typeName = nodeIdentifier.ToString(); //Not ToFullString() as it may include spaces and break.
if (!typeName.EndsWith(AssemblyChangesLoader.ClassnamePatchedPostfix))
{
typeName += AssemblyChangesLoader.ClassnamePatchedPostfix;
}
return AddRewriteCommentIfNeeded(
node.ReplaceToken(nodeIdentifier, SyntaxFactory.Identifier(typeName)),
$"{nameof(RecordeRewriter)}:{nameof(AdjustRecordName)}"
);
}
}
}
Thanks - that looks good, and targetting just VisitRecordDeclaration
makes it quite isolated. It'd be great if you can create a PR and I'll merge that in