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

Line wrapping breaks at the wrong place when the line is indented. #691

Closed
seronal opened this issue May 15, 2015 · 4 comments
Closed

Line wrapping breaks at the wrong place when the line is indented. #691

seronal opened this issue May 15, 2015 · 4 comments
Milestone

Comments

@seronal
Copy link

seronal commented May 15, 2015

The problem is related to indentation. It's as if js-beautify "double counts" indent spaces while calculating where to break the line.

If you beautify the following code with "wrap_line_length" set to 80, dummy2 and dummy3 lines get wrapped even though they should not. There are the same number of B's in each string as the number of indent spaces in that line and if you remove all the B's from those strings they won't be wrapped.

This bug does not exist in version 1.5.1, but it exists in every version released since.

var dummy = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA';

function dummyFunction() {
    var dummy2 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';

    function dummyFunction2() {
        var dummy3 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
    }
}

Output from jsbeautifier.org

var dummy = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA';

function dummyFunction() {
    var dummy2 =
        'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';

    function dummyFunction2() {
        var dummy3 =
            'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
    }
}
@redapple
Copy link
Contributor

I sort of managed to follow this with a bunch of print statements.
It looks related to TK_WORD, 'dummyFunction' + TK_START_EXPR, '(' starting an expression handled in handle_start_expr(), but also indenting (see start_of_statement call at the beginning of handle_start_expr())

In [5]: t = r'''var dummy = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA';

function dummyFunction() {
    var dummy2 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';

    function dummyFunction2() {
        var dummy3 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
    }
}'''

In [6]: print jsbeautifier.beautify(t, opts)
__character_count=0
__character_count=0
('TK_RESERVED', 'var')
start_of_statement: self.last_type 'TK_START_BLOCK', self.flags.last_text ''; self.flags.mode 0
print_token_line_indentation
Output::set_indent(level=0)
OutputLine::set_indent(level=0)
__character_count(0) = __parent.baseIndentLength(0) + level(0) * __parent.indent_length(4)
__character_count(3) += 3 (`var`)
('TK_WORD', 'dummy')
start_of_statement: self.last_type 'TK_RESERVED', self.flags.last_text 'var'; self.flags.mode 0
start_of_statement: self.indent()
indent: self.flags.indentation_level=1
proposed_line_length 8 = self.output.current_line.get_character_count() 3 + len(current_token.text) 5
print_token_line_indentation
__character_count(4) += 1 (` `)
__character_count(9) += 5 (`dummy`)
('TK_EQUALS', '=')
start_of_statement: self.last_type 'TK_WORD', self.flags.last_text 'dummy'; self.flags.mode 1
print_token_line_indentation
__character_count(10) += 1 (` `)
__character_count(11) += 1 (`=`)
('TK_STRING', "'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'")
start_of_statement: self.last_type 'TK_EQUALS', self.flags.last_text '='; self.flags.mode 1
proposed_line_length 78 = self.output.current_line.get_character_count() 11 + len(current_token.text) 67
proposed_line_length += 1 (79)
print_token_line_indentation
__character_count(12) += 1 (` `)
__character_count(79) += 67 (`'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'`)
('TK_SEMICOLON', ';')
start_of_statement: self.last_type 'TK_STRING', self.flags.last_text "'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'"; self.flags.mode 1
restore_mode: self.flags.indentation_level=0
print_token_line_indentation
__character_count(80) += 1 (`;`)
('TK_RESERVED', 'function')
__character_count=0
__character_count=0
start_of_statement: self.last_type 'TK_SEMICOLON', self.flags.last_text ';'; self.flags.mode 0
print_token_line_indentation
Output::set_indent(level=0)
OutputLine::set_indent(level=0)
__character_count(0) = __parent.baseIndentLength(0) + level(0) * __parent.indent_length(4)
__character_count(8) += 8 (`function`)
('TK_WORD', 'dummyFunction')
start_of_statement: self.last_type 'TK_RESERVED', self.flags.last_text 'function'; self.flags.mode 0
print_token_line_indentation
__character_count(9) += 1 (` `)
__character_count(22) += 13 (`dummyFunction`)
('TK_START_EXPR', '(')
start_of_statement: self.last_type 'TK_WORD', self.flags.last_text 'dummyFunction'; self.flags.mode 0
start_of_statement: self.indent()
indent: self.flags.indentation_level=1
proposed_line_length 23 = self.output.current_line.get_character_count() 22 + len(current_token.text) 1
print_token_line_indentation
__character_count(23) += 1 (`(`)
handle_start_expr: self.indent() #2
indent: self.flags.indentation_level=2
('TK_END_EXPR', ')')
restore_mode: self.flags.indentation_level=1
print_token_line_indentation
__character_count(24) += 1 (`)`)
('TK_START_BLOCK', '{')
print_token_line_indentation
__character_count(25) += 1 (` `)
__character_count(26) += 1 (`{`)
handle_start_block: self.indent()
indent: self.flags.indentation_level=2
('TK_RESERVED', 'var')
start_of_statement: self.last_type 'TK_START_BLOCK', self.flags.last_text '{'; self.flags.mode 0
__character_count=0
print_token_line_indentation
Output::set_indent(level=2)
OutputLine::set_indent(level=2)
__character_count(8) = __parent.baseIndentLength(0) + level(2) * __parent.indent_length(4)
__character_count(11) += 3 (`var`)
('TK_WORD', 'dummy2')
start_of_statement: self.last_type 'TK_RESERVED', self.flags.last_text 'var'; self.flags.mode 0
start_of_statement: self.indent()
indent: self.flags.indentation_level=3
proposed_line_length 17 = self.output.current_line.get_character_count() 11 + len(current_token.text) 6
print_token_line_indentation
__character_count(12) += 1 (` `)
__character_count(18) += 6 (`dummy2`)
('TK_EQUALS', '=')
start_of_statement: self.last_type 'TK_WORD', self.flags.last_text 'dummy2'; self.flags.mode 1
print_token_line_indentation
__character_count(19) += 1 (` `)
__character_count(20) += 1 (`=`)
('TK_STRING', "'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB'")
start_of_statement: self.last_type 'TK_EQUALS', self.flags.last_text '='; self.flags.mode 1
proposed_line_length 82 = self.output.current_line.get_character_count() 20 + len(current_token.text) 62
proposed_line_length += 1 (83)
__character_count=0
print_token_line_indentation
Output::set_indent(level=3)
OutputLine::set_indent(level=3)
__character_count(12) = __parent.baseIndentLength(0) + level(3) * __parent.indent_length(4)
__character_count(74) += 62 (`'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB'`)
('TK_SEMICOLON', ';')
start_of_statement: self.last_type 'TK_STRING', self.flags.last_text "'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB'"; self.flags.mode 1
restore_mode: self.flags.indentation_level=2
print_token_line_indentation
__character_count(75) += 1 (`;`)

@bitwiseman bitwiseman added this to the v1.6.0 milestone May 27, 2015
@bitwiseman
Copy link
Member

@redapple - I'm having trouble following your print statement debugging, but thanks for trying.
@seronal - The word wrap is very approximate. In this case, it looks like a bug that can/should be fixed. I'd just like to set expectation here that, even when this particular case is fixed, there will be many other cases where wrap will occur sooner than you might expect.

@seronal
Copy link
Author

seronal commented May 27, 2015

@bitwiseman I totally understand that wrap might occur sooner than the exact column if line length is longer than "wrap_line_length". However, this is a different case. Per my understanding of the specification; a line should never be wrapped if it's shorter than "wrap_line_length", i.e. there should not be any approximation/uncertainty about whether to wrap, but only where to wrap.

@bitwiseman bitwiseman modified the milestones: v1.5.6, v1.6.0 May 27, 2015
@bitwiseman
Copy link
Member

a line should never be wrapped if it's shorter than "wrap_line_length"

No, unfortunately. At this time, we have a process of indenting where we indent all blocks, and when a block closes we look at the contents and remove the redundant indents. This results in some cases where we will indent before expected.

Note: this was not one of those cases and is now fixed.
But...

(function() {
    var dummy2 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';
    var dummy3 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
})

// This has an intermediate state that looks like this
(function() {
        var dummy2 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';
        var dummy3 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
})

// At the end the enclosing (), the beautifier removes the extra indent, 
// but it doesn't currently have the functionality to go back and re-evaluate all the line wraps.
// The result is this:
(function() {
    var dummy2 =
        'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';
    var dummy3 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
})

That is unfortunate, but a known issue -

This on the other hand, is a new bug.

dummyFunction(function() {
    var dummy2 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';
    var dummy3 = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
})

// Second line should really not wrap, but does... 
// There still an extra intermediate indent in here somewhere.
dummyFunction(function() {
    var dummy2 =
        'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBB';
    var dummy3 = 
        'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABBBBBBBB';
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants