Annotated SyntaxList

[jaredpar]

No description provided.

[jaredpar]

@dotnet/roslyn-compiler PTAL

[cston]

SyntaxNode? [](start = 28, length = 11)

When is this null? #Resolved

[jaredpar]

Most obvious place is here:

        public SyntaxList(IEnumerable<TNode>? nodes)
            : this(CreateNode(nodes)!)

Which ... I should really remove the ! from cause it's unnecessary. #Resolved

[sharwell]

Done with 9/14 files

[sharwell]

💡 Consider splitting the expression list.ItemInternal(i) to a local, then asserting that it is not null. #WontFix

[sharwell]

❔ Is it possible for this to reach external code and still be null?

[jaredpar]

Not that I'm aware of. It's an implementation detail of this type.

---

In reply to: 362963462 [](ancestors = 362963462)

[sharwell]

💡 Consider refactoring this method:

var listNode = builder?.ToListNode();
if (listNode is null)
{
  return null;
}

return new SeparatedSyntaxList<TNode>(new SyntaxNodeOrTokenList(listNode.CreateRed(), 0));

Applies to methods above too. #Resolved

[jaredpar]

That would change the return type of the method to be a nullable struct. That's not a trivial change here.

---

In reply to: 362969063 [](ancestors = 362969063)

[jaredpar]

Did u mean return default?

---

In reply to: 362974659 [](ancestors = 362974659,362969063)

[sharwell]

Yes I meant return default. ToListNode only returns null if the list is empty, so the checks can all be combined. #Resolved

[sharwell]

💡 The implementation allows builder to be null. I accounted for this in my suggested refactoring.

Applies to all methods in this type. #Resolved

[sharwell]

Review complete. If the changes in SyntaxListBuilderExtensions.cs are not implemented as part of this pull request, please file an issue for them since it seems like a good improvement to make.

[333fred]

            return _builder == null;

This implies _builder should be annotated. Why is it not? #Resolved

---

Refers to: src/Compilers/Core/Portable/Syntax/SyntaxListBuilder`1.cs:30 in 8e507a5. [](commit_id = 8e507a5, deletion_comment = False)

[cston]

        }

Should this use var listNode = builder?.ToListNode(); for consistency with extension methods below?

---

Refers to: src/Compilers/Core/Portable/Syntax/SyntaxListBuilderExtensions.cs:14 in 8e507a5. [](commit_id = 8e507a5, deletion_comment = False)

[jaredpar]

            return _builder == null;

It's a good question. One of the tricky parts of nullability is how we treat reference fields inside struct. Do we annotate for the default case or the case where we assume it's always properly constructed. Our struct implementation also suggest that we struggle with this type of decision. So far I've been leaning towards "annotate for default". In this case I just missed it. Will update.

Overall though I don't think this type of scenario is cut and dry.

---

In reply to: 570725573 [](ancestors = 570725573)

---

Refers to: src/Compilers/Core/Portable/Syntax/SyntaxListBuilder`1.cs:30 in 8e507a5. [](commit_id = 8e507a5, deletion_comment = False)