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

no-multi-assign option only when declaring variables. #12545

Closed
zepumph opened this issue Nov 7, 2019 · 14 comments · Fixed by #14185
Closed

no-multi-assign option only when declaring variables. #12545

zepumph opened this issue Nov 7, 2019 · 14 comments · Fixed by #14185
Assignees
Labels
accepted archived due to age enhancement rule

Comments

@zepumph
Copy link

zepumph commented Nov 7, 2019

What rule do you want to change?

no-multi-assign
https://eslint.org/docs/rules/no-multi-assign

Does this change cause the rule to produce more or fewer warnings?

For this rule, it produces fewer.

How will the change be implemented? (New option, new default behavior, etc.)?

I think it should be an option, something like onlyForVariableDeclation of type {boolean}. When true, no-multi-assign will only trigger when multi-assign occurs when a variable is being declared. That said. I don't have a firm grasp on all the possible option schemas available, and I'm sure there is likely a better one for this case.

Please provide some example code that this change will affect:

// incorrect usage `no-multi-assign` with { "onlyForVariableDeclation": true }
const tree = { numberOfLeaves: 9 };
const numberOfLeaves = tree.numberOfLeaves = 10; // would error out

// correct usage `no-multi-assign` with { "onlyForVariableDeclation": true }
const x = {};
const y = {};
x.hi = y.hi = "salutations"; // acceptable

What does the rule currently do for this code?

It errors out both when declaring variables in the statement as well as when no declaring.

What will the rule do after it's changed?

You will have the option to only error out when declaring variables

Are you willing to submit a pull request to implement this change?

I am, though this is likely a simple change, and I have not set up the eslint dev environment before. If someone else feels passionately, then likely I won't be the most efficient implementer.

Here is a copy of the rule, outfitted with a first pass at the changes, for review:

no-multi-assign.js with added option
/**
 * @fileoverview Rule to check use of chained assignment expressions
 * @author Stewart Rand
 */

"use strict";


//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

module.exports = {
  meta: {
    type: "suggestion",

    docs: {
      description: "disallow use of chained assignment expressions",
      category: "Stylistic Issues",
      recommended: false,
      url: "https://eslint.org/docs/rules/no-multi-assign"
    },

    schema: [ {
      type: "object",
      onlyForVariableDeclation: {
        type: "boolean",
        default: false
      }
    }
    ]
  },

  create( context ) {

    const options = context.options[ 0 ] || {};
    const onlyForVariableDeclation = options.onlyForVariableDeclation;
    const disallowedParents = [ "VariableDeclarator" ].concat( onlyForVariableDeclation ? [] : [ "AssignmentExpression" ] );


    //--------------------------------------------------------------------------
    // Public
    //--------------------------------------------------------------------------

    return {
      AssignmentExpression( node ) {
        if ( disallowedParents.indexOf( node.parent.type ) !== -1 ) {
          context.report( {
            node,
            message: "Unexpected chained assignment."
          } );
        }
      }
    };

  }
};
@zepumph zepumph added enhancement rule triage labels Nov 7, 2019
@kaicataldo kaicataldo added evaluating and removed triage labels Nov 9, 2019
@kaicataldo
Copy link
Member

kaicataldo commented Nov 9, 2019

Thanks for the proposal. Could you clarify what problem you’re trying to solve?

@zepumph
Copy link
Author

zepumph commented Nov 11, 2019

Yes of course.

The project I work for called PhET Interactive Simulations (https://github.com/phetsims/) has decided on code style that allows multi-assign when when not declaring a variable, but disallows multi-assign when declaring.

So this would be flagged as an error:

 var x = someObject.y = 'string value';

Where this would be allowed:

x = {};
y = {};
x.something = y.something = 'string value';

Currently there is no way to make the eslint rule no-multi-assign differentiate between those two, but when I looked at the rule source code, it looks like it could be straight forward to supply only one or the other (since each has a different node parent type), potentially based on a boolean option.

@kaicataldo
Copy link
Member

kaicataldo commented Nov 14, 2019

Thanks for the explanation. This seems like a reasonable request to me, though I wonder if we can come up with an option name that's more along the lines of ignoreNonDeclaration - i.e. adding it as a black list options rather than a white list. Implementation-wise, it doesn't make a difference, but it prevents us from backing ourselves into a corner if we add options in the future.

@kaicataldo
Copy link
Member

kaicataldo commented Nov 14, 2019

Whoops, sorry, didn't mean to close this 😅

@kaicataldo kaicataldo reopened this Nov 14, 2019
@zepumph
Copy link
Author

zepumph commented Nov 14, 2019

Brainstorming a few more possibilities here:

  • What about white list options that default to true? Since currently the rule supports two "modes" of sorts, failing out on assignment in declaration, and out of declaration, we could have two options that default to true, like:
    • {boolean} prohibitOnDeclaration=true
    • {boolean} prohibitOnAssignment=true
      Then you can opt out of either. That feels pretty robust, and could allow for future adaptation.
  • (I think this is a worse idea) What about a single string option like {'onDeclaration'|'onAssignment'|'all'} specificationMode='all'. This would be more prohibitive and less future proof.

@kaicataldo
Copy link
Member

kaicataldo commented Nov 15, 2019

I think that generally makes more sense and falls in line with some of other rules. We could create flags with the AST node type as the key (both defaulting to true):

{
  VariableDeclaration: boolean;
  AssignmentExpression: boolean;
}

@kaicataldo kaicataldo self-assigned this Nov 15, 2019
@kaicataldo
Copy link
Member

kaicataldo commented Nov 15, 2019

I'll champion this. Let's see what the rest of the team has to say.

@mdjermanovic
Copy link
Member

mdjermanovic commented Nov 15, 2019

This rule is in the Stylistic Issues category. In my opinion:

  • a = b = 1; is mostly a stylistic issue.
  • var a = b = 1; is more a possible error than a stylistic issue, as it looks like the intention to declare b as well.

An error on b will be usually reported by no-undef (and no-implicit-globals if enabled). But if it happens that b already exists in an outer scope, no-multi-assign would be the only rule to report this.

So I'm 👍 for an option to disable this check in statements as a user's preference because it allows the user to keep the rule enabled to report possible errors in declarations.

@zepumph
Copy link
Author

zepumph commented Nov 15, 2019

Thanks for looking into it you two!

@anikethsaha
Copy link
Member

anikethsaha commented Jul 20, 2020

+1 for ignoreNonDeclaration

@anikethsaha
Copy link
Member

anikethsaha commented Jul 23, 2020

Marking this as accepted as there are 3 👍 from team members.

From above comments, marking this with ignoreNonDeclaration as an option name.

@anikethsaha anikethsaha added accepted and removed evaluating labels Jul 23, 2020
@anikethsaha
Copy link
Member

anikethsaha commented Jul 23, 2020

@zepumph you can submit a PR if you want else let me know.

@t-mangoe
Copy link
Contributor

t-mangoe commented Feb 20, 2021

@anikethsaha @zepumph
Hi. I'm working on this issue.
If you like, can I send pull request?

I added ignoreNonDeclaration option, and made it possible to accept multiple assignments if the variable has already been declared.

@t-mangoe
Copy link
Contributor

t-mangoe commented Mar 6, 2021

I will try sending a pull request.

mdjermanovic pushed a commit that referenced this issue Apr 3, 2021
… (#14185)

* Update: add an option to ignore non declaration (refs #12545)

* Doc: add description of the option (refs #12545)

* Chore: reflect review comments (refs #12545)

* Chore: modify the document according to comments (refs #12545)
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 1, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age label Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age enhancement rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants