Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File types binding #60977

Merged
merged 19 commits into from
May 28, 2022
Merged

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Apr 27, 2022

Related to #60819

I recommend disabling whitespace diffs when reviewing.

Implements the following sections of the spec (dotnet/csharplang#6011):

No accessibility modifiers can be used in combination with file on a type.

file types can only appear in signatures or as base types of other file types

@jcouv @cston for review

I'm particularly wondering if the way I'm doing lookup, and the way I'm getting the "associated syntax tree" from a binder, is correct.

@RikkiGibson RikkiGibson marked this pull request as ready for review May 9, 2022 21:43
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 9, 2022 21:43
@RikkiGibson RikkiGibson requested review from jcouv and cston May 9, 2022 21:44
@jcouv jcouv self-assigned this May 9, 2022
}

// PROTOTYPE(ft):
// ensure nested types within 'file' types are either disallowed or treated as implicitly 'file'
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test accessibility restrictions on record positional members as well.

Copy link
Member

@jcouv jcouv May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more test ideas:

  1. Do we have tests with aliases of file-types or including file-types?
  2. File-type containing extension methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test accessibility restrictions on record positional members as well.

See test PrimaryConstructor_01.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test Alias_01.

@@ -2281,6 +2299,11 @@ private BestSymbolInfo GetBestSymbolInfo(ArrayBuilder<Symbol> symbols, out BestS

private static BestSymbolLocation GetLocation(CSharpCompilation compilation, Symbol symbol)
{
if (symbol is SourceMemberContainerTypeSymbol { IsFile: true })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is SourceMemberContainerTypeSymbol { IsFile: true }

Will we get here with a member of a file type? If so, should BestSymbolLocation.FromFile be returned in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might, in some cases where the member access is not qualified. The only relevant case I can think of, though, involves using static.

// File1.cs
using static C.D;

M();

file class C
{
    public class D
    {
        public static void M() { }
    }
}

// File2.cs
class C
{
    public class D
    {
        public static void M() { }
    }
}

I'll add the test. Let me know if any other scenarios come to mind that might hit this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this test and it "just worked". I suspect this is because we "broke the tie" when binding the using static C.D, and binding the call is just a simple lookup within that.

@RikkiGibson RikkiGibson mentioned this pull request May 27, 2022
83 tasks
return foundType is not null;
}

public static bool IsFileTypeInSeparateFileFrom(this SourceNamedTypeSymbol possibleFileType, SourceNamedTypeSymbol otherSymbol)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we think this will get re-used? If not consider making this a local function at single call-site

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this might get reused in another place where we report redundant symbols, but it didn't end up being the case. Moving to local function.

{
public class C
{
void M(Outer outer) { } // ok
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider adding void M2(C c) { } // ok as well in this test (unless covered elsewhere)

}
""";

var comp = CreateCompilation(new[] { source1, main }); // expectedOutput: 2
Copy link
Member

@jcouv jcouv May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// expectedOutput: 2

nit: I would have used a PROTOTYPE marker, but I think it's fine. Let's remember to hunt down every such instance

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 14)

var model = comp.GetSemanticModel(tree, ignoreAccessibility: true);
var cReference = tree.GetRoot().DescendantNodes().OfType<MemberAccessExpressionSyntax>().Last();
var info = model.GetTypeInfo(cReference);
Assert.Equal(expectedSymbol.GetPublicSymbol(), info.Type);
}
}
Copy link
Member

@cston cston May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we test base references, in error scenarios?

// file1.cs
file class Base
{
    public Base(int i) { }
}
partial class Derived : Base
{
}

// file2.cs
partial class Derived
{
    Derived() : base(0) { }
}
// file1.cs
file class Base
{
    public virtual void F1() { }
}
partial class Derived : Base
{
    public override void F1() { }
}

// file2.cs
partial class Derived
{
    void F2() { base.F1(); }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to add these onto the test plan and add them in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a line to #60819.

{
static void Main()
{
Program.C.M();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Program.

The qualifier isn't needed is it? Just confirming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meaning of the program isn't changed, but the method of binding is changed. Here we lookup the symbols directly in the Program type rather than in a binder which contains the members of Program. I think I added this to address the issue discussed in #60977 (comment).

@RikkiGibson
Copy link
Contributor Author

Failing tests are unrelated to my change. #61017

@RikkiGibson RikkiGibson merged commit 2226d1a into dotnet:features/file-types May 28, 2022
@RikkiGibson RikkiGibson deleted the ft-binding branch May 28, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants