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

Work Breakdown for "Proposed changes for deconstruction, declaration expressions, and discards" #14832

Closed
32 of 33 tasks
gafter opened this issue Oct 31, 2016 · 6 comments · Fixed by #15842
Closed
32 of 33 tasks

Comments

@gafter
Copy link
Member

gafter commented Oct 31, 2016

This is a breakdown of the work needed to implement the changes described in #14794 ("Proposed changes for deconstruction, declaration expressions, and wildcards")

Language design loose ends

  • We need to specify more precisely the interaction of existing declarations (named _) with wildcards. (1 LDM meeting or less)
  • We should record the spec decisions in a suite of tests that describe the principal design points. (< 1 day)
  • There are some parsing ambiguity resolutions that need to be decided and implemented. (< 1 day)

Syntax and parser changes

  • The syntax changes described in that issue need to be made in Syntax.xml (< 1 day)
  • Corresponding changes to the parser need to be made. (< 1 day)
    • For the purpose of parsing, a declaration expression should be accepted in a tuple literal
  • Tests need to be modified and augmented to reflect these changes (< 1 day)
  • There are no parser changes needed to support wildcards.

Semantic analysis changes

  • Semantic analysis should reject a declaration expression in a tuple literal except in a deconstruction context, which is (< 1 day)
    • On the left-hand-side of an assignment expression
    • In the variable of a foreach statement (which is now an ExpressionSyntax)
    • A declaration expression is permitted in an out argument
    • Other contexts will be an error, but may be supported in the future
  • Binding of a deconstruction assignment needs to be modified to support a mixture of existing variables (i.e. lvalue expressions), wildcards, and declaration expressions. (< 4 days)
  • (requires LDM review) Pattern, out, and deconstruction variables that are declared but not used should cause a warning to be produced (because if you didn't want to use the variable you could have used a wildcard). (< 1 day)

Scoping changes

There are a number of changes related to the scoping of _ as an identifier.

  • Remove support for the deconstruction declaration statement. It is a special case of an expression statement. (< 1 day)
  • When binding an lvalue for a deconstruction or out argument, if we are looking up the simple identifier _ and find nothing, we treat it as a target-typed wildcard rather than an error. (< 1 day)
    • This probably requires a new bound node, so that its type can be inferred for the purposes of lowering and the SemanticModel
  • When a declaration expression for an out argument or in a tuple expression declares _, we treat it as a wildcard.
  • Permit multiple declarations for _ lambda parameters, but they introduce nothing into scope. (< 2 days) Permit discards in lambda parameters #16255
  • Debuggers should ignore wildcards, except lambda parameters should be displayed as _. (< 2 days)
  • There are probably other scope-related behavioral changes for _ that need to be specified and implemented. (< 4 days)
  • Although it is proposed to support wildcards in lambda parameters such as (_,_)=>1, I propose we not implement that in the first iteration of changes. It is a separable and compatible change that can be done in the next iteration.

Lowering changes

  • Remove support for deconstruction declaration. (< 1 day)
  • Generalize the assignment expression to allow declaration expressions and wildcards inside a tuple lvalue. (< 1 day when done together with related binding changes)
  • We should be careful to retain the optimization that we do not construct a tuple result of a deconstruction assignment when it is in a context that discards the value.

Semantic Model changes

The behavior for the semantic model needs to be decided in a design meeting among compiler folks and IDE people.

  • GetTypeInfo on a NameSyntax that is a wildcard should give the type of the discarded value. (< 1 day)
  • We need to define the behavior for a declaration expression whose identifier is _.
  • GetTypeInfo on a declaration expression that is not a tuple should give the type of the variable.
  • GetTypeInfo on a declaration expression that is a tuple (e.g. var (x, (y, _))) should give the type of the consumed tuple value.
  • We need to define and implement the behavior for a tuple designator, like (y, _) in var (x, (y, _)).
  • Need tests to confirm that SemanticModel behavior (e.g. GetTypeInfo) works for non-declaration discard expressions in deconstruction expression, foreach loop, and for loop initializer.

IDE impact (per @CyrusNajmabadi)

IDE impact for existing features is fairly superficial. The IDE tests are mostly behavioral, and virtually all of them of them remain correct in their current form. Most of the changes will just "work right" once the SemanticModel has been adjusted to implement behavior for the new features. (< 2 days)

The rename refactoring will have to be modified to prevent you from renaming something to _ unless it has no references.

There will be some opportunities for new features. For example, fixing a "pattern/out/deconstruction variable declared but not used" warning would be a small refactor that changes it to use a wildcard (i.e. rename it to _). That is a new feature, optional, and not required as part of this work.

Other items to follow-up on:

  • Consider renaming IDiscardedSymbol to IDiscardSymbol

Overall estimate

This assumes work done by area owners

Language design wrapup < 1 day
Syntax changes < 3 days
Scope changes < 4 days
Semantics changes < 4 days
Lowering changes < 4 days
IDE changes < 2 days

If the work herein is shared among area owners (e.g. scope work done or assisted by @AlekseyTs, deconstruction lowering done or assisted by @jcouv) this work can be largely completed over the duration of one sprint; it is less than one half of the team's work for the duration of a sprint, with a few smaller issues trailing into the following sprint.

@CyrusNajmabadi
Copy link
Member

Discussed this with @gafter . We have test coverage for the areas this would hit. Expected fallout cost on the IDE side should be minimal. We may need additional logic to detect new error conditions (for example, warning when renaming something to '_'). We may also want new features (for example, a feature that suggests using a wildcard when a pattern variable is unused).

We may also want something provided from the semantic/symbol model to identify if '_' is a wildcard. That will likely make a bunch of things easier (for example, disallow 'rename' on a wildcard, etc. etc.).

@alrz
Copy link
Contributor

alrz commented Oct 31, 2016

Now that the wildcard is settled for vnext, I'd like to know about the final decision toward using wildcard and default case in switch. Restating from #8818 discussion,

default shouldn't be synonymous with case *. The default case should be considered the last case regardless of where it appears in lexical order, thus retaining the behavior that it has today.

Whereas with case _ subsumption rules will be applied.

Per #11438 (comment)

We never warn about a default case being unreachable or unnecessary.

So it will be allowed to use wildcard with a default case in a single switch statement?

@gafter
Copy link
Member Author

gafter commented Nov 1, 2016

@alrz I don't understand your question. A default: case doesn't have an identifier that could be a wildcard. Are you wondering if both a default: case and a case var _: would be permitted in the same switch? If so, the answer is the same as if the identifier is something other than _: yes.

@jcouv jcouv self-assigned this Nov 1, 2016
@gafter gafter modified the milestones: 2.0 (RC.2), 2.0 (RTM) Nov 1, 2016
@gafter
Copy link
Member Author

gafter commented Nov 1, 2016

See also #14862

@jaredpar jaredpar changed the title Work Breakdown for "Proposed changes for deconstruction, declaration expressions, and wildcards" Work Breakdown for "Proposed changes for deconstruction, declaration expressions, and dicards" Nov 16, 2016
@jaredpar jaredpar changed the title Work Breakdown for "Proposed changes for deconstruction, declaration expressions, and dicards" Work Breakdown for "Proposed changes for deconstruction, declaration expressions, and discards" Nov 16, 2016
@gafter
Copy link
Member Author

gafter commented Nov 17, 2016

jcouv pushed a commit that referenced this issue Dec 12, 2016
…ds. (#15548)

* Combine deconstruction assignment and declaration, and support discards.

- Combine deconstruction assignment and declaration, and support discards.
- Add wildcards.work.md to track outstanding work.
- Bind each type syntax once in a deconstruction.
- Because tuples may contain declarations, adjust lambda disambiguation
  and adjust parsing of argument lists.
- Diagnose tuple element names on the left of a deconstruction.
- Add relational operators to disambiguating tokens in 7.5.4.2

* Disallow deconstruction declarations except at the statement level.
This is now a semantic restriction (until we decide to remove it).

* Revise logic to detect `var` in a declaration expression.
Plus other changes per code review.

* Add a test that GetTypeInfo on a discard expression doesn't crash.
* Small changes per code review.
* Add (skipped) test for var invocation in parens.
* Rename "Discarded" to "Discard"
* Changes recommended via code review.
* Minor changes to the handling of declaration expressions per code review.
* Addressing blocking feedback while Neal is OOF

Fixes #14794
Fixes #14832
@jcouv jcouv reopened this Dec 16, 2016
@jcouv jcouv modified the milestones: 2.1, 2.0 (RTM) Jan 11, 2017
@jcouv
Copy link
Member

jcouv commented Jan 24, 2017

I'll close this umbrella issue. The only open item is already tracked by an individual issue:

@jcouv jcouv closed this as completed Jan 24, 2017
@jcouv jcouv added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 3 - Working labels Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment