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

Add proposal for partial type inference #7582

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Conversation

TomatorCZ
Copy link

No description provided.

@TomatorCZ TomatorCZ requested a review from a team as a code owner October 7, 2023 11:25
@333fred 333fred requested a review from jcouv October 9, 2023 16:35
@RikkiGibson RikkiGibson self-assigned this Oct 9, 2023
@jcouv
Copy link
Member

jcouv commented Oct 16, 2023

Sorry for the delay. We've focused on wrapping up C# 12 bugs. We'll look in a few weeks. Thanks

## Summary
[summary]: #summary

Partial type inference introduces a syntax skipping obvious type arguments in the argument list of
Copy link
Member

@jcouv jcouv Nov 2, 2023

Choose a reason for hiding this comment

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

obvious

nit: instead of "obvious" consider "inferrable" (also applies below) #Closed

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 33b71f0

> Example
>
> ```csharp
> var wrappedData = Create(new MyData());
Copy link
Member

@jcouv jcouv Nov 2, 2023

Choose a reason for hiding this comment

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

nit: Do we need this line to illustrate the point?
I think the line below (return new Wrapper<T>(item);) is central and sufficient #Closed

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 5e59e96

> class Wrapper<T> { public Wrapper(T item) { ... } }
> ```

2. Cases where the method type inference would be weak. (Using type info from target type, or type arguments' constrains)
Copy link
Member

@jcouv jcouv Nov 2, 2023

Choose a reason for hiding this comment

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

Suggested change
2. Cases where the method type inference would be weak. (Using type info from target type, or type arguments' constrains)
2. Cases where the method type inference would be weak. (Using type info from target type, or type constraints)

typo: constrains -> constraints, also applies in other locations below #Closed

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 33b71f0

- The semantics of an identifier named `_` depends on the context in which it appears:
- It can denote a named program element, such as a variable, class, or method, or
- It can denote a discard ([§9.2.9.1](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/variables.md#9291-discards)).
- **It can denote an inferred type argument avoiding specifying type arguments which can be inferred by the compiler.**
Copy link
Member

@jcouv jcouv Nov 2, 2023

Choose a reason for hiding this comment

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

Can be simplified

Suggested change
- **It can denote an inferred type argument avoiding specifying type arguments which can be inferred by the compiler.**
- **It can denote a type argument to be inferred.**
``` #Closed

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 33b71f0

> Specification: Original section changed in the following way

* We change the meaning of the content of *type_argument_list* in two contexts.
* [Constructed types](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/types.md#84-constructed-types) occuring in [*object_creation_expression*](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/expressions.md#128162-object-creation-expressions)
Copy link
Member

Choose a reason for hiding this comment

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

As a general comment, I appreciate the references back to the standard/spec. The format you adopted in other sections (copy a paragraph from original spec and add some highlighted edits) is really nice. But when I look at "Type arguments" section, it's not clear which section of the spec is being edited.

Maybe you mean to edit section 8.4.2 ("type arguments")? That seems to be the right place to introduce inferred_type_argument into the type_argument_list grammar.
(If so, then the references to "constructed types" (8.4) and "object_creation_expression" could be removed)

Below is a possible grammar change to introduce inferred_type_argument:

type_argument_list
  : '<' type_arguments '>'
  ;
  
type_arguments // modified
  : type_argument (',' type_arguments)
  | inferred_type_argument (',' type_arguments)
  ;   

type_argument
  : type
  ;

inferred_type_argument // new
  : '_'
  ;

Then we can say that inferred_type_argument is only allowed in the contexts that you listed.

We still need to resolve the issue of back compat:

public class _ { }
public class C<T> 
{
    public void M(C<_> x) { } // allowed today
}

We'll need to decide whether the presence of an existing type named "" in scope affects the interpretation of "" in type argument lists, or whether "" always means "inferred type argument" (ie. we take a compat break, maybe warn on types named "").

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by 6039b9f.

> {
> void M()
> {
> new C1<>( ... ); // Refers generic_inferred type C1<T>
Copy link
Member

@jcouv jcouv Nov 2, 2023

Choose a reason for hiding this comment

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

T, T1 and T2 don't exist here.
Reading ahead to the "Object creation expressions" section, I believe the comments in this snippet are not what you intended and some actual types should be inferred from inspecting candidate constructors. #Closed

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit b6081e3

proposals/partial-type-inference.md Outdated Show resolved Hide resolved
> ```

The first improvement, which would improve the method type inference algorithm, has a significant disadvantage of the breaking change.
On the other hand, the second improvement, which would enable specifying some of the method's type arguments, does not influence old code, solves problems regarding the "all or nothing" principle, and reduces the first weakness.
Copy link
Member

@jaredpar jaredpar Nov 3, 2023

Choose a reason for hiding this comment

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

I'm unclear what the "all or nothing" principle means here. #Resolved

Copy link
Author

Choose a reason for hiding this comment

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

Line 58 explains "all or nothing" principle. The method type inference infers either all of the type arguments or nothing (e.g. we can't hint the compiler some of the type arguments).

Choose a reason for hiding this comment

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

In line 68, wouldn't it even be possible to infer twice: test<TestCaseDefault<_>, _>(new MyData()); ?

Copy link
Author

@TomatorCZ TomatorCZ Feb 20, 2024

Choose a reason for hiding this comment

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

Sure, it would be possible, I will fix it during the weekend. (Commit 98d835e)
Thank you

- The semantics of an identifier named `_` depends on the context in which it appears:
- It can denote a named program element, such as a variable, class, or method, or
- It can denote a discard (§9.2.9.1).
- \***It can denote an inferred type argument avoiding specifying type arguments which can be inferred by the compiler.**
Copy link
Member

@jaredpar jaredpar Nov 3, 2023

Choose a reason for hiding this comment

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

Rather than it can I'd prefer the language it will. Essentially when flipping to /langversion:X where X is the release that introduces this feature, I'd prefer the meaning to be unambiguous. That is for the case M<_, object>(42, null), the _ unambiguously means an inferred type parameter.

Yes this would be breaking change to developers who defined class _ { } but that seems like a minority case. It in the same ballpark as class record which we broke when introducing records.
#Resolved

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 33b71f0

@TomatorCZ
Copy link
Author

Thank you for all comments. I will revise the proposal during the weekend.

@TomatorCZ
Copy link
Author

Done with the first round of revisions.

@TomatorCZ
Copy link
Author

Hi,

I implemented a prototype which passes Test.cmd -testCompilerOnly tests(except the ones, which fail in main on which the change is rebased on). See fork. The implementation is rather a proof of concept than ready to production code.

Probably the most interesting file contains tests for the partial type inference.

The implementation contains the mentioned design in the proposal without 2. and 3. parts mentioned here

>
> ```csharp
> object data = database.fetch();
> int count = data.Field("count"); // Error, method type inference can't infer the return type. We have to specify the type argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider breaking the examples up across more lines in general. For example, here put the comment above the statement instead of trailing it on the same line.

GitHub requires to horizontally scroll the code samples at about 115 characters in a line, and when the samples are presented to LDM, maybe even fewer, as the font size will be larger.

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit e973d80

+ ;
```

* When a type with `_` identifier is presented in the scope where **inferred_type_argument** is used, a warning should appear since the **inferred_type_argument** hides the type's name causing a breaking change.
Copy link
Contributor

Choose a reason for hiding this comment

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

How should the user resolve this warning if it appears?

  • It will mostly occur in pre-existing code. If user desires the "old meaning", then perhaps changing _ to @_ is the proper advice.
  • If the warning occurs, but inferred_type_argument is actually desired, the user probably needs to suppress the warning, or possibly adjust using-directives etc.

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 4485ab9


> Specification: Original section changed in the following way

We change the [type inference](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/expressions.md#1263-type-inference) as follows.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the below section is entirely new. If the below section is entirely new, please state here that it is being added to the type inference section.

If there is a combination of adding new language and updating existing language, please use bold for the newly-added parts, and non-bold for the pre-existing parts.

Copy link
Author

Choose a reason for hiding this comment

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

Should by fixed by commit 9dfa24f

proposals/partial-type-inference.md Outdated Show resolved Hide resolved
proposals/partial-type-inference.md Outdated Show resolved Hide resolved
proposals/partial-type-inference.md Outdated Show resolved Hide resolved

#### Type inference algorithm change

> Specification: Original section changed in the following way.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use block quotes only for when we are quoting from another source. Also, let's indicate here that the specification change is followed by an explanation of the change.

Basically, looking at this spec language on its own, without the explanation, is more challenging, and it may be reasonable for the reader to simply skim the spec change, then review the explanation, then finally go back to the spec change in order to fully understand.

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed by commit 7aa277e

@TomatorCZ
Copy link
Author

Done with the second round of revisions.

TomatorCZ and others added 5 commits March 13, 2024 15:45
1. top level _ in method inv and creating object expr
2. nested _ in method inv and creating object expr
@@ -160,7 +160,7 @@ The initial set of candidate methods for is changed by adding new condition.
- If `F` is generic and `M` has no type argument list, `F` is a candidate when:
- Type inference (§12.6.3) succeeds, inferring a list of type arguments for the call, and
- Once the inferred type arguments are substituted for the corresponding method type parameters, all constructed types in the parameter list of `F` satisfy their constraints ([§8.4.5](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/types.md#845-satisfying-constraints)), and the parameter list of `F` is applicable with respect to `A` (§12.6.4.2)
- **If `F` is generic and `M` has a type argument list containing at least one *inferred_type* or *partial_inferred_type*, `F` is a candidate when:**
- **If `F` is generic and `M` is *partial_inferred*, `F` is a candidate when:**
- **Method type inference (See the [Type Inference] section) succeeds, inferring the type arguments list for the call, and**
Copy link
Contributor

Choose a reason for hiding this comment

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

Was Type Inference meant to be a link here?

Copy link
Author

@TomatorCZ TomatorCZ Apr 16, 2024

Choose a reason for hiding this comment

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

Comment on lines 176 to 179
- If `T` is a *value_type* and `A` is not present:
- **The *object_creation_expression* is a default constructor invocation.**
- **If the type is *partial_inferred*, Constructor type inference (See the [Type Inference] section) of the default constructor occurs to determine closed type. If it succeeded, construct the type using the inferred type arguments. If it failed the binding-time error occurs.**
- **If the type inference above succeeded or the type is not inferred, the result of the *object_creation_expression* is a value of (constructed) type `T`, namely the default value for `T` as defined in [§8.3.3](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/types.md#833-default-constructors).**
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm conflicted about having these lines here because:

  • they are out of date (new ValueType() can be invocation of a parameterless (explicitly declared) struct constructor).
  • the described inference will never succeed. Perhaps in future if target type contributes to inference, but not with this version of the feature.

I think for the purposes of this feature speclet we could delete these lines and just remove "Otherwise," from the "Otherwise, if T is a type parameter"... below.

F<C<_>, int>( ... ); // C<_> represents an inferred type.
new C<C<_>, int>( ... ); // C<C<_>, int> represents an inferred type.
C<_> temp = ...; // _ nor C<_> doesn't represent an inferred type.
new _( ... ); // _ doesn't represent an inferred type.
Copy link
Contributor

Choose a reason for hiding this comment

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

LDM will want to talk in more details about the breaking change here, exactly what will _ mean in each context, and so on.

I feel like we should consider breaking _ whenever it is used in a type position. Even if it makes no sense here, I think it is better for readability of the code if _ in new _() does not refer to a different thing than _ in new C<_>(), in the same scope.

This search gives me some confidence that the breaks caused by this approach are acceptable: https://grep.app/search?q=public.%2A%28class%7Cstruct%7Cinterface%7Cenum%29%20_%5Cs&regexp=true&filter[lang][0]=C%23


* A type is said to be *partial_inferred* if it is *inferred_type_placeholder* or *partial_inferred_type*.

* When a type with the `_` identifier is presented in the scope where *inferred_type_placeholder* is used, a warning should appear since the *inferred_type_placeholder* hides the type's name causing a breaking change.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can, we should start issuing a warning on declaring or using a type named _ in the previous language version to let people know this change is coming. (where changing to @_ suppresses the warning and doesn't get flagged in analyzers for detecting unnecessary @.)

Hopefully we can separate this question of whether/how to adjust behavior of _ for discards, while leaving space for them to eventually both work in a coherent manner. (IIRC, fixing the quirks around _ is one of the more often discussed breaking changes, see #7189)


We change the [compile-time checking](https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/expressions.md#1265-compile-time-checking-of-dynamic-member-invocation).

- First, if `F` is a generic method and type arguments were provided, then if the method group is *partial_inferred*, an binding error occurs since it is not supported in the runtime binding.
Copy link
Contributor

Choose a reason for hiding this comment

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

We will likely decide to perform a partial inference at compile time when a single candidate is found for the dynamic invocation. It's not urgent to revise this proposal to get this precisely right. Certain details of this may be subject to change. See https://github.com/dotnet/csharplang/blob/main/proposals/params-collections.md#dynamic-vs-static-binding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Active/Investigating
Development

Successfully merging this pull request may close these issues.

5 participants