Fix Issue #63993 by improving out-of-place binding diagnostic and reflecting 'var' or 'let' binding pattern

[Rajveer100]

Fixes Issue #63993

Added a new 'DescriptivePatternKind' declaration with its methods and functionality similar to 'DescriptiveDeclKind', thereby allowing binding pattern check in the appropriate places for 'var' or 'let' and improving the overall diagnostics.

[Rajveer100]

@hamishknight
In my understanding, these are the following additions that remain:

• StringRef Pattern::getDescriptivePatternKindName(DescriptivePatternKind K)
• DescriptivePatternKind Pattern::getDescriptiveKind() const
• +Misc.

[hamishknight]

Definitely on the right track! The remaining items on your list sound good to me

[Rajveer100]

Done!

[hamishknight]

Looks great! I have a few more comments, but otherwise this looks good to me. A couple more things that also need doing before we can land this:

1. Can you run git clang-format over this change? If not already installed, you should be able to install clang-format with homebrew.
2. You'll need to update any tests that have had their diagnostics changed. Off the top of my head, I know there are a couple of cases in test/Sema/redeclaration-checking.swift that will need changing to expect 'let' binding pattern instead of 'var' binding pattern (and there's a FIXME comment you can now remove).

[Rajveer100]

Looks great! I have a few more comments, but otherwise this looks good to me. A couple more things that also need doing before we can land this:

1. Can you run git clang-format over this change? If not already installed, you should be able to install clang-format with homebrew.
2. You'll need to update any tests that have had their diagnostics changed. Off the top of my head, I know there are a couple of cases in test/Sema/redeclaration-checking.swift that will need changing to expect 'let' binding pattern instead of 'var' binding pattern (and there's a FIXME comment you can now remove).

Hey there, I will make the changes, thanks for the review!

[Rajveer100]

2. You'll need to update any tests that have had their diagnostics changed. Off the top of my head, I know there are a couple of cases in test/Sema/redeclaration-checking.swift that will need changing to expect 'let' binding pattern instead of 'var' binding pattern (and there's a FIXME comment you can now remove).

I don't find any such case in the mentioned file, no 'FIXME' comment as well. The only similar cases I observed were the ones with the 'redeclaration' issue or similar.

[Rajveer100]

Apart from that, I have done the changes!

[hamishknight]

@Rajveer100 This one here:

https://github.com/apple/swift/blob/70b982f9ca6d6ce87df12e2a5d5654d02514b4f4/test/Sema/redeclaration-checking.swift#L126-L130

and:

https://github.com/apple/swift/blob/70b982f9ca6d6ce87df12e2a5d5654d02514b4f4/test/Sema/redeclaration-checking.swift#L150-L152

[Rajveer100]

Hey! Looks like my codebase isn't updated since a while, cause the number of LOC in the file referenced is barely around ~120.

Probably I should git pull.

[hamishknight]

Ah that makes sense, be sure to git pull --rebase to avoid adding a merge to the branch

[Rajveer100]

Well, that's done, hope there aren't any more test files which need changes!

[Rajveer100]

Mistakenly had closed this pull request for a second!
Apologies for that!

[hamishknight]

Wonderful, thank you!

[hamishknight]

Well, that's done, hope there aren't any more test files which need changes!

Let's find out :)

@swift-ci please test

[Rajveer100]

Observed an issue!

I ran the tests specifically for the test file in context:

/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:113:6: error: expected error not produced
  // expected-error@-1 {{invalid redeclaration of 'x'}}
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:114:6: error: expected note not produced
  // expected-note@-2 {{'x' previously declared here}}
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:117:6: error: expected error not produced
  // expected-error@-1 {{invalid redeclaration of 'x'}}
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:118:6: error: expected note not produced
  // expected-note@-2 {{'x' previously declared here}}
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:122:8: error: expected error not produced
    // expected-error@-1 {{invalid redeclaration of 'x'}}
~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:123:8: error: expected note not produced
    // expected-note@-2 {{'x' previously declared here}}
~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:130:6: error: expected error not produced
  // expected-error@-2 {{invalid redeclaration of 'x'}}
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/rajveersingh/Desktop/swift-project/swift/test/Sema/redeclaration-checking.swift:131:6: error: expected note not produced
  // expected-note@-3 {{'x' previously declared here}}
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Rajveer100]

Failed Tests (1):
  Swift(macosx-arm64) :: Sema/redeclaration-checking.swift


Testing Time: 0.41s
  Excluded: 8811
  Failed  :    1

4 warning(s) in tests

[hamishknight]

@Rajveer100 Did you rebuild after rebasing your branch? Those test failures sound like you have a build that was done before #63994

[Rajveer100]

Ah yes, forgot about that!
Let me do that! :)

[Rajveer100]

A question!
Could you briefly differentiate between normal tests and smoke tests, also when they are used and when not.

[hamishknight]

Sure, the details are documented in https://github.com/apple/swift/blob/main/docs/ContinuousIntegration.md#pull-request-testing. In short, a full test runs more configurations (e.g different targets, different optimization levels) than a smoke test. Personally I tend on the side of doing a full test unless the change is fairly trivial (e.g something like a light refactoring), though honestly it's probably overkill for a change like this.

[Rajveer100]

All checks passed!