typescript-book icon indicating copy to clipboard operation
typescript-book copied to clipboard

fixed documentation of flags enum

Open ronnieoverby opened this issue 5 years ago β€’ 10 comments

the code before did not correctly check for the presence of a flag

ronnieoverby avatar Oct 10 '18 17:10 ronnieoverby

Thanks :rose:. But I see no error here : https://goo.gl/eaNuo5

Will reopen once you share an error please :rose:

basarat avatar Oct 11 '18 03:10 basarat

There's a logic error. Humor me and change line 25 to CanFly.

ronnieoverby avatar Oct 11 '18 04:10 ronnieoverby

Done. Still matches my expectations:

enum AnimalFlags {
    None           = 0,
    HasClaws       = 1 << 0,
    CanFly         = 1 << 1,
}
type Animal = {
    flags: AnimalFlags
}

function printAnimalAbilities(animal: Animal ) {
    var animalFlags = animal.flags;
    if (animalFlags & AnimalFlags.HasClaws) {
        console.log('animal has claws');
    }
    if (animalFlags & AnimalFlags.CanFly) {
        console.log('animal can fly');
    }
    if (animalFlags == AnimalFlags.None) {
        console.log('nothing');
    }
}

let animal: Animal = { flags: AnimalFlags.None };
printAnimalAbilities(animal); // nothing
animal.flags |= AnimalFlags.CanFly; // You asked to change
printAnimalAbilities(animal); // Correctly prints: animal can fly
animal.flags &= ~AnimalFlags.HasClaws;
printAnimalAbilities(animal); // Correctly prints: animal can fly
animal.flags |= AnimalFlags.HasClaws | AnimalFlags.CanFly;
printAnimalAbilities(animal); // Correctly prints: animal has claws, animal can fly

basarat avatar Oct 11 '18 04:10 basarat

That's what I get for debugging in my head. Standby.

ronnieoverby avatar Oct 11 '18 04:10 ronnieoverby

The point I'm trying to make is easier to see when you extrapolate the idea of checking for the presence of specific bits by creating a general purpose function to do so.

Take a look:

enum AnimalFlags {
    None = 0,
    HasClaws = 1 << 0,
    CanFly = 1 << 1,
    CanFlyWithClaws = CanFly | HasClaws
}
type Animal = {
    flags: AnimalFlags
}

type HasFlag = (bitfield: number, flag: number) => boolean;

function hasFlagBad(bitfield: number, flag: number): boolean {
    // flawed technique of checking for presence of bits
    // doesn't work reliably
    // I created the pull request because
    // as a general technique this will introduce subtle bugs

    // rely on truthiness of bitwise and.... ugh no thanks
    if (bitfield & flag)
        return true;

    return false;
}

function hasFlagGood(bitfield: number, flag: number): boolean {
    // typical technique of checking for presence of bits
    // works reliably in all cases
    // if I were writing a book, I'd steer people towards this
    // as a general technique because it will avoid subtle bugs
    // caused by the other technique
    return (bitfield & flag) === flag;
}

function printAnimalAbilities(animal: Animal, hasFlag: HasFlag) {

    var animalFlags = animal.flags;

    if (animalFlags === AnimalFlags.None) {
        console.log('nothing');
    }
    else if (hasFlag(animalFlags, AnimalFlags.CanFlyWithClaws)) {
        console.log('animal can fly and has claws');
    }
    else if (hasFlag(animalFlags, AnimalFlags.HasClaws)) {
        console.log('animal only has claws');
    }
    else if (hasFlag(animalFlags, AnimalFlags.CanFly)) {
        console.log('animal only can fly');
    }
}

// using the unreliable technique
{
    let animal: Animal = { flags: AnimalFlags.None };
    printAnimalAbilities(animal, hasFlagBad); // nothing (OK)
    animal.flags |= AnimalFlags.HasClaws;
    printAnimalAbilities(animal, hasFlagBad); // animal only has claws (OK)
    animal.flags &= ~AnimalFlags.HasClaws;
    printAnimalAbilities(animal, hasFlagBad); // nothing (OK)
    animal.flags |= AnimalFlags.CanFlyWithClaws;
    printAnimalAbilities(animal, hasFlagBad); // animal only has claws (WRONG)
}

console.log('====================');

// using the reliable technique
{

    let animal: Animal = { flags: AnimalFlags.None };
    printAnimalAbilities(animal, hasFlagGood); // nothing (OK)
    animal.flags |= AnimalFlags.HasClaws;
    printAnimalAbilities(animal, hasFlagGood); // animal only has claws (OK)
    animal.flags &= ~AnimalFlags.HasClaws;
    printAnimalAbilities(animal, hasFlagGood); // nothing (OK)
    animal.flags |= AnimalFlags.CanFlyWithClaws;
    printAnimalAbilities(animal, hasFlagGood); // animal can fly and has claws (YES!)

}

ronnieoverby avatar Oct 11 '18 05:10 ronnieoverby

The issue is not the checking of the flags, but the fact that your code sample uses else if instead of if.

basarat avatar Oct 11 '18 05:10 basarat

The issue is not the checking of the flags, but the fact that your code sample uses else if instead of if.

That misses the point I'm making.

TL;DR The technique you show only works for checking the presence of a single bit. The technique doesn't work for checking for multiple bits.

Obviously it's your book and you can teach the world whatever you like. But, do you at least understand what I'm trying to say?

Today, I eye-witnessed first-hand someone that looked at that sample in your book and tried to apply it in their own code, but they checked for more than 1 bit and they had a bug in their program. 🌹

ronnieoverby avatar Oct 11 '18 05:10 ronnieoverby

The technique you show only works for checking the presence of a single bit. The technique doesn't work for checking for multiple bits

If a & b checks for one then a & b && a & c checks for two. Its fairly intuitive and works correctly.

Example:

enum AnimalFlags {
    None = 0,
    HasClaws = 1 << 0,
    CanFly = 1 << 1,
}
type Animal = {
    flags: AnimalFlags
}

function printAnimalAbilities(animal: Animal) {
    var animalFlags = animal.flags;

    // Only on two flags
    if (animalFlags & AnimalFlags.CanFly
        && animalFlags & AnimalFlags.HasClaws) {
        console.log('animal can fly and has claws');
    }
}

let animal: Animal = { flags: AnimalFlags.None };
printAnimalAbilities(animal); // nothing
animal.flags = AnimalFlags.HasClaws;
printAnimalAbilities(animal); // nothing
animal.flags = AnimalFlags.CanFly;
printAnimalAbilities(animal); // nothing
animal.flags |= AnimalFlags.HasClaws;
printAnimalAbilities(animal); // animal can fly and animal has claws

but they checked for more than 1 bit and they had a bug in their program

No code is bullet proof from mistakes :rose:

More

My pattern comes from the compiler source code itself e.g. https://github.com/Microsoft/TypeScript/blob/6bd1da20c978c6e652ec827dadecfad46d91a29f/src/services/refactors/extractSymbol.ts#L347 So essentially I am documenting what the TypeScript team itself :rose:

What I have is hasThisFlag. What you have in hasFlagGood is really onlyHasExactlyTheseFlags. It is definitely worth documenting in addition what I have right now so opening for my own mental note :rose:

basarat avatar Oct 11 '18 05:10 basarat

If a & b checks for one then a & b && a & c checks for two. Its fairly intuitive and works correctly.

a & b && a & c is only correct while the symbols b and c represent values with a single positive binary digit.

Said another way, when the values represented by b or c are today in the sequence 2^n, but tomorrow they are not, what works correctly today, will silently fail tomorrow when the enum has changed. The compiler won't help you. That's the worst kind of programming error.

For example...

enum AnimalFlags {
    None = 0,
    HasClaws = 1 << 0,
    CanFly = 1 << 1,
}

... later becomes...

enum AnimalFlags {
    None = 0,
    HasClaws = 1 << 0,
    HasWings = 1 << 1,
    IsAerodynamicAndLight = 1 << 2,
    CanFly = HasWings | IsAerodynamicAndLight 
}

... and the technique if(flags & AnimalFlags.CanFly) becomes a regret, especially in complex systems where silent logic errors cost money. Let's pray that no one programs an aircraft in Typescript/JS. πŸ”₯ ✈️ πŸ”₯

My pattern comes from the compiler source code itself e.g. https://github.com/Microsoft/TypeScript/blob/6bd1da20c978c6e652ec827dadecfad46d91a29f/src/services/refactors/extractSymbol.ts#L347 So essentially I am documenting what the TypeScript team itself

For the reasoning just given, they ought to avoid that technique. That same enum has members outside of the 2^n sequence: https://github.com/Microsoft/TypeScript/blob/dd9b8cab34a3e389e924d768eb656cf50656f582/src/compiler/types.ts#L547

What you have in hasFlagGood is really onlyHasExactlyTheseFlags

That doesn't seem right. Here's how I would implement something called onlyHasExactlyTheseFlags

function onlyHasExactlyTheseFlags(bitfield: number, flag: number): boolean {
    return bitfield === flag;
}

No code is bullet proof from mistakes

I don't know anything about that. But I do know what this does and doesn't do:

function hasFlag(bitfield: number, flag: number): boolean {
    return (bitfield & flag) === flag;
}

.NET has a function HasFlag that is implemented in the same safe way: https://docs.microsoft.com/en-us/dotnet/api/system.enum.hasflag?view=netframework-4.7.2#remarks

The Bottom Line

Truthy and Falsy strikes again.


By the way, please don't feel attacked. There's always the risk of these discussions feeling very adversarial. I only want to help your readers avoid making mistakes.

Thanks for your efforts. typescript-book is certainly a great resource.

ronnieoverby avatar Oct 11 '18 15:10 ronnieoverby

I don’t feel attacked. You have been very tech focused πŸŒΉβ€οΈπŸ™‡πŸ»β€β™‚οΈ

basarat avatar Oct 11 '18 21:10 basarat