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

Diagnostic for use of plain protocol name in type position should suggest some or any #68284

Open
hborla opened this issue Sep 1, 2023 · 16 comments · May be fixed by #74197
Open

Diagnostic for use of plain protocol name in type position should suggest some or any #68284

hborla opened this issue Sep 1, 2023 · 16 comments · May be fixed by #74197
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler itself diagnostics QoI Bug: Diagnostics Quality of Implementation existentials Feature: values of types like `any Collection`, `Any` and `AnyObject`; type-erased values fix-its Feature: diagnostic fix-its generics Feature: generic declarations and types good first issue Good for newcomers opaque types Feature → types: opaque types swift 5.9 type checker Area → compiler: Semantic analysis types Feature: types

Comments

@hborla
Copy link
Member

hborla commented Sep 1, 2023

The following code is invalid:

protocol P {
  associatedtype A
}

func generic(value: P) {}

The compiler currently produces the error message Use of protocol 'P' as a type must be written 'any P' with a fix-it to insert the any keyword.

However, a different (and often better!) fix here is to use the some keyword. The error message should be re-worded to prompt the programmer to consider any or some, and there should be two notes, each with a fix-it to insert any or some, respectively.

@hborla hborla added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels good first issue Good for newcomers diagnostics QoI Bug: Diagnostics Quality of Implementation fix-its Feature: diagnostic fix-its type checker Area → compiler: Semantic analysis and removed triage needed This issue needs more specific labels labels Sep 1, 2023
@AnthonyLatsis AnthonyLatsis added compiler The Swift compiler itself swift 5.9 opaque types Feature → types: opaque types existentials Feature: values of types like `any Collection`, `Any` and `AnyObject`; type-erased values types Feature: types generics Feature: generic declarations and types labels Sep 1, 2023
@Dhandeep10
Copy link

@hborla
sir, what is meant here, can you please elaborate, so that I can contribute accordingly.

@taevonlewis
Copy link

I’m currently working on fixing this bug in diagnostics as part of the swift mentorship program

@Dhandeep10
Copy link

I too want to contribute and get started, I am new to open source, so want to learn and explore this.

@saehejkang
Copy link
Contributor

@taevonlewis are you still working on this issue?

@taevonlewis
Copy link

@saehejkang Yes, I am.

@li3zhen1
Copy link
Contributor

Hi @taevonlewis, are you still working on this?

@saehejkang
Copy link
Contributor

saehejkang commented May 1, 2024

@AnthonyLatsis can this be reassigned due to its lack of activity?

EDIT

Even if this issue is still being worked on, I found some great notes on Diagnostics.md to help me wrap my head around how fix-itsand diagnostics are created/tested. Just had a question to test my knowlede with the current fix-it.

protocol P {
  associatedtype A
}
func generic(value: P) {} // expected-error {{use of protocol 'P' as a type must be written 'any P'}}
                        // expected-note@-1 {{Replace 'P' with 'any P'}} {{21-21= any P}}

Is this close to how the verifier should be written? This is not passing the -verify flag when I run it locally, so would love to know what it should be.

Let's say I get it to pass verification, but I change the fix-it to actually now be some P instead of any P. I understand the verification will fail, but will the console output what it should be below (show the diff).

@AnthonyLatsis
Copy link
Collaborator

AnthonyLatsis commented May 1, 2024

-verify mode diagnoses both grammatical issues and mismatches — with details on actual vs. expected diagnostics (fix-its).

@AnthonyLatsis
Copy link
Collaborator

@taevonlewis Are you fine with passing this over to Saehej?

@taevonlewis
Copy link

Yes, they can take over this issue

@saehejkang
Copy link
Contributor

with details on actual vs. expected diagnostics (fix-its).

With the -verify flag added to the swift-frontend build arguments, I get this output (no details on actual), just an error that the note is not produced.

/Documents/xcode/SwiftCompiler/SwiftCompiler/main.swift:71:28: error: expected note not produced
                        // expected-note@-1 {{Replace 'P' with 'any P'}} {{21-21=any P}}
~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@AnthonyLatsis
Copy link
Collaborator

AnthonyLatsis commented May 1, 2024

Right, there is no actual note emitted on the specified line (and at all), so there is nothing to compare your expectation with, just the fact that it was not fulfilled.

@saehejkang
Copy link
Contributor

there is no actual note emitted on the specified line

Screenshot 2024-05-01 at 10 36 58 AM

Is the fix-it not an expected note? Maybe I wrote one of these wrong

// expected-error {{use of protocol 'P' as a type must be written 'any P'}}
// expected-note@-1 {{Replace 'P' with 'any P'}} {{21-21= any P}}

@saehejkang
Copy link
Contributor

We want to make updates to this diagnostic I found below. I was thinking to add a new variable Type and pass in the 'some P' to the error, but I had some questions about where that comes from.

ERROR(existential_requires_any,none, "use of %select{protocol |}2%0 as a type must be written %1", 
(Type, Type, bool))

On this line in TypeCheckType the diagnostic is used and passes in the Type returned by getDeclaredExistentialType(). Correct me if I am wrong, but I believe that is the value 'any P'. The ExistentialType is spelled with the keyword any as it says so here in Types.

My thought process was to look for something similar that uses the keyword some and I landed on looking into OpaqueTypes. Is there something similar to getDeclaredExistentialType() that pretty much fetches the value 'some P'? I delved deeper into OpaqueTypes and feel I need use something like this .

@AnthonyLatsis
Copy link
Collaborator

AnthonyLatsis commented May 2, 2024

Is the fix-it not an expected note?

A fix-it by itself is not a diagnostic message. It is a text substitution descriptor (a range + a replacement string) that is attached to and emitted with a diagnostic message. Custom fix-it descriptions are implemented by attaching the fix-it to a note rather than the primary warning/error. The fix-it on the screenshot is attached to an error; its description was autogenerated by Xcode.


I suggest hardcoding the some keyword instead of attempting to construct a semantic type that will print as some P. An opaque parameter type — the alternative we want to suggest — is no more than syntactic sugar for the declaration and use of a generic parameter, and does not have a separate syntax-preserving representation in the type system. OpaqueTypeArchetypeType is for opaque result types.

@AnthonyLatsis
Copy link
Collaborator

@hborla Do you reckon we should also suggest both options in positions where some X is an opaque result type? Also, we should probably refrain from suggesting some if the parent declaration is ABI-public and library evolution is enabled. That could cause an ABI break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler itself diagnostics QoI Bug: Diagnostics Quality of Implementation existentials Feature: values of types like `any Collection`, `Any` and `AnyObject`; type-erased values fix-its Feature: diagnostic fix-its generics Feature: generic declarations and types good first issue Good for newcomers opaque types Feature → types: opaque types swift 5.9 type checker Area → compiler: Semantic analysis types Feature: types
Projects
None yet
6 participants