A potential bug in the JS code analyser


This a bug? I’ve declared the function, and yet the analyser says I haven’t. This warning disappears if I explicitly “declare” a function, either with the arrow or regular syntax.

It seems that the analyser doesn’t recognize .bind returning a function. How would we go about solving this?

It is indeed incorrect for the analyzer to mark this as a missing function (apart from the fact that it uses the incorrect term method here, which is from a previous iteration of this exercise). The problem here is that extractFunctions does not recognise the bind operation as a way to generate a function.

The preferred solution would not be to make that function understand bind, but rather make the analyzer for this exercise recognise this specific usage.

Whilst it’s absolutely valid to solve the solution this way, and using bind (and call, and apply) are powerful and common tools in JavaScript, solving this particular solution using that construct is not optimal in terms of maintainability and readability. The cognitive complexity of the solution including bind is much higher than one that actually declares a function.

// Function declaration
export function colorCode(color) {
  return COLORS.indexOf(color)
}

// Anonymous function declaration + variable assignment
export const colorCode = (code) => COLORS.indexOf(color)

// Member call + variable assignment
export const colorCode = Array.prototype.indexOf.bind(COLORS)

In the same way that the code highlighter here doesn’t understand colorCode is now a function (how could it? bind can be arbitrary, and/or overwritten), a reader, especially if less experienced, will have a very hard time understanding what is going on.

If you want the bind variant to not trigger this message, we’ll accept a PR to resolve this on the analyzer repository.

A problem I felt with the arrow function version is that functionally it didn’t seem to make much sense - create a function that does nothing but call another function with the parameters passed in. This might be useful if we gave a more appropriate name to our function, but as it’s arrow here, that too doesn’t apply. However, you’re right - it’s much simpler and better to use that. The reason I included this that it didn’t seem to be commonly available in the community solutions, and I wanted to show a different, shorter, albeit more complicated, method of solving this exercise.

I’d be glad to open a PR, so I’ll start work on that, then. Thanks!

Binding a function does exactly the same :slight_smile:

…that of course is your prerogative :wink: . Like I said: there is functionally nothing wrong with the solution.

Looking forward to this. If you could ping me on GitHub (@sleeplessbyte) and post the link here, I’ll make sure it’s looked at / merged!

Sure, I’m starting out, and will let you know when it’s complete! It’s my first time contributing to an analyser, so it’ll take some time.

1 Like

Of course! If you run into issues or have questions, feel free to ask them here!

Hi SleeplessByte,

I’m currently stuck. This is my code, at line 391 of the constructer function of ResistorColorSolution:

So the bind line in the source code is replaced with an arrow function version, which, as you’ve said, is the same. However, I need to change this in the program parameter too, as the functions are extracted from that and not source. Apart from program being readonly (which I’ll change), how do I edit lines of code on the program - or even better, reevaluate program based on the new source?

Thanks!

Awesome effort to have the picture and the copy/paste version for folks that may want to explore the code in their editors!

Removed my version, for the more trustworthy code paste posted hereafter.

1 Like

Hi Victor, thanks for your assistance! I apologize for posting a photo, it was rather inconsiderate of me. Here’s the code, with correct indentation:

const bindRegex = new RegExp(`${EXPECTED_METHOD}\s=.*indexOf\.bind\(COLORS\)`)
const indexOfBind = source.split('\n').findIndex(line => bindRegex.test(line))
if (indexOfBind) {
  let source_list = source.split('\n')
  source_list.splice(indexOfBind, 1, `${EXPECTED_METHOD} = color => COLORS.indexOf(color)`)
  source = source_list.join('\n')
}
1 Like

Apart from program being readonly (which I’ll change),

Please do not change this. It’s readonly on purpose.

or even better, reevaluate program based on the new source ?

You cannot.

What you’re trying to do is rewrite anything with bind in a way so that the analyzer understands. That’s actually a smart way to approach it, but unfortunately not supported. We’d need something like a pre-run hook in order to facilitate this.

What I recommend you do instead is use https://astexplorer.net/ (pick @typescript-eslint/parser) to generate the AST from your solution. Try to find the relevant part that calls .bind. It is likely a MemberCall or something like that.

I think there where to code tries to find the function, it should check if bind is used and if so, it uses that function instead. I think this will be, albeit harder than “changing the source”, less error prone, more maintainable and something that doesn’t require a change in how the internals of the analyzer work.

Let me know if you are stuck with this.

I’ll check out your resource, thanks!

1 Like

Need some help. This is my code currently:

constructor(public readonly program: Program, source: string) {
    this.source = new Source(source)

    const functions = extractFunctions(program)
    const exports = extractExports(program)

    let line = program.body.find(line => line.declaration.declarations[0].id.name === 'colorCode')
    if (line && line.declaration.declarations[0].init?.callee?.property?.name === 'bind') {
        let colorCodeIndex = exports.findIndex(export_ => export_.local === 'colorCode')
        exports[colorCodeIndex].kind = 'function';
    }

So I find the line in which the declaration is made, and then change the kind of the export. The next step should be to change the node of the export to this:

image
I have two problems here, though. First, how do I create a ExportNamedDeclaration with such values? Is there any way to copy over from the AST Explorer? Second, if I do use such a method (static creation), values such as range will be wrong - is that fine?

I don’t really like this solution, is there a more succinct and less problematic way of approaching this?

I’ll have a look after the weekend. Ping me next week if you haven’t heard anything!

@SleeplessByte, any update? Thanks in advance!

1 Like

Apologies!

I have the last live show on May 13th. I know this is another week away, but could you pin me in the week of the 15th? Happy to look over it then with full attention. I will have all the time in the world then!!

Sure, there’s no hurry!

1 Like

@SleeplessByte, I hope your live show went well! What did you demonstrate?

It was not a demonstration but the final of a month-long music competition for best live act.

Went well!

I have not yet found the time writing the code for the analyzer, but it’s next on my list.

1 Like