24 points

Interesting question. I’d be comfortable up to level 2 in this list, after which I want to have my eyes on the changes. Even where code is functionally or semantically equivalent, style can make a lot of difference for comprehension and maintainability.

permalink
report
reply
6 points

I’d agree, for the same reasons. Communicating intent is definitely one of the main things that separates mediocre from amazing developers (and software can’t check that).

It’s interesting to consider a tool that does all of levels 1-3 (and more) as a way to verify that a style refactoring hasn’t changed logic. I assume that’s what they meant when they wrote “modifications that were supposed to be no-ops but aren’t”.

permalink
report
parent
reply
4 points

4 is sheer madness. 1 is common sense. 2 is just the cooler version of 1.

I’ve always found hardcoded style to be an obnoxious and counterproductive paradigm. It’s the text editor’s job to handle line wrapping, and there’s no reason a coding editor shouldn’t be able to format code intelligently. I hate hard line breaks that do not have meaning. Not everybody is using the same size windows! It’s 2024! We have the technology!

permalink
report
reply
7 points

The example for 2 isn’t good. Seemingly superfluous commas, brackets, and escaped newlines can be useful and even important for clean maintenance.

The solution to the whitespace gripe is strictly enforced formatting standards with a git hook running a manually invokable script.

permalink
report
parent
reply
1 point

Yeah but sometimes you do get meaningless changes that aren’t just whitespace even with auto formatters. For example if you change the indentation on some code and that causes it to wrap an expression.

permalink
report
parent
reply
1 point

How is that not whitespace?

permalink
report
parent
reply
1 point
*

The solution to the whitespace gripe is strictly enforced formatting standards with a git hook running a manually invokable script.

Throwing a linter into the pipeline just hardcodes the formatting at that point in the pipeline. That doesn’t really solve the issue, which is that style is not a one-size-fits-all concept, and displaying text appropriately is really the job of a text editor. To quote PEP 8, “default wrapping in most tools disrupts the visual structure of the code”. In other words, “most tools” suck at displaying code, because they are not language-aware. That’s the real problem. Hardcoding style is a workaround, not a solution.

That said, I wouldn’t consider intelligent editors to be a replacement for formatting standards, either. Ideally my text editor would display my Python code the way I like it, and then save to disk in accordance to PEP 8.

permalink
report
parent
reply
5 points

Why would there be one answer to this? I’d probably use all the available levels depending on the situation, in the same way I’d use --word-diff or -b in git when I need help understanding a complex change.

permalink
report
reply
36 points
*

Using a tool like this to hide sections of code presented for review places a lot of trust in the automation. If Mallory were to discover a blind spot in the semantic diff logic, she could slip in a small change for eventual use in an exploit, and it would never be seen by another human.

For example, consider this part of the exploit used in the recent xz backdoor. In case you don’t see the problem, here’s the fix.

Rather than hiding code from review, if a tool figured out a way to use semantic understanding to highlight code that might be overlooked by a human (and should therefore be reviewed more carefully), it could conceivably help find such things.

permalink
report
reply
5 points
*

I don’t have an opinion on the topic but I see a blind spot in your argument, so I have to be that kind of person … 🥺

One could use the exact same example to argue that humans are very bad at parsing code (especially if whitespace kicks in). In that regard a tool that allows them to reason on a standardized representation of the AST can be a protection against a whole class of attacks.

permalink
report
parent
reply
10 points
*

That’s not a blind spot in my comment. See my final paragraph.

It’s only one sentence. Maybe it was easy to miss. :)

permalink
report
parent
reply
1 point

I like the idea, but I can’t come up with any method that won’t devolve into most reviewers only checking the highlighted parts tbh.

permalink
report
parent
reply
1 point

Oh yeah, so I’m that other kind of guy 🥺

I kinda like your idea, but I think it can be difficult to detect some confusing situations. I think it would be a better idea, but I don’t think it’s a full replacement.

permalink
report
parent
reply
0 points

If Mallory were to discover a blind spot in the semantic diff logic

This is a very big stretch IMO. That xz change wasn’t actually the exploit, it was just used to make the exploit less detectable. And it was added by people with commit access so it didn’t even have to go through code review.

On top of that, code review is not magic. It’s easy to get bugs past it hiding in plain sight (if that wasn’t the case Linux would be bug free!).

Can you think of an actually realistic example?

permalink
report
parent
reply
16 points

I was into this until I realized that it’s not open source and not even available outside of vscode and GitHub web

permalink
report
reply

Programming

!programming@programming.dev

Create post

Welcome to the main community in programming.dev! Feel free to post anything relating to programming here!

Cross posting is strongly encouraged in the instance. If you feel your post or another person’s post makes sense in another community cross post into it.

Hope you enjoy the instance!

Rules

Rules

  • Follow the programming.dev instance rules
  • Keep content related to programming in some way
  • If you’re posting long videos try to add in some form of tldr for those who don’t want to watch videos

Wormhole

Follow the wormhole through a path of communities !webdev@programming.dev



Community stats

  • 3K

    Monthly active users

  • 895

    Posts

  • 7.8K

    Comments