JohnEarnest/Octo

Misleading errors when constant definitions incomplete

pushfoo opened this issue · 4 comments

Octo build used: Current web IDE build
Browser used: Chrome 86.0.4240.111 (Official Build) (64-bit), Windows

I haven't tried this using the nodejs cli.

The assembler sometimes gives incorrect or misleading errors when a constant definition isn't fully completed. Beginners will get confused by this, so it's worth clarifying the errors.

Two simple examples are listed below, but I think are still some strange and more severe instances of this problem that I still need to create succinct examples for.

Example 1

The code below produces line 13: Undefined name ':'. instead of a clearer message indicating that a definition wasn't fully closed.

: filler_label_1
	v2 := v2
return

:macro NOOP {
	v0 := v0
}

# bad definition
:const UNDEFINED_CONST

# this is only an example to take up space
: filler_label_2
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
return

: main # actual game implementation would go here 
	hires
	
	: game_loop

	filler_label_1
	filler_label_2

	jump game_loop

Example 2

In this version, the example returns line 7: Undefined name ':macro'.

: filler_label_1
	v2 := v2
return

:const UNDEFINED_CONST

:macro NOOP {
	v0 := v0
}



# this is only an example to take up space
: filler_label_2
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
return

: main # actual game implementation would go here 
	hires
	
	: game_loop

	filler_label_1
	filler_label_2

	jump game_loop

	v0 := key

With the updated compiler, your examples would produce error messages like:

Line 13: Expected a constant name, but found the keyword ":macro". Missing a token?

The current changes seem to be an increase in clarity without increasing compiler complexity. I'd be in favor of even clearer messages that point out mistakes in more depth, but i can understand if you want to keep the code as simple as possible.

Also, the code below produces an error of line 26: Undefined name 'EXAMPLE_CONSTANT'. It's true that this is an undefined constant, but I'm not sure why the compiler doesn't flag the earlier malformed constant definition. It might be worthwhile to block the usage of reserved keywords as label names.

: const EXAMPLE_CONSTANT 5

:macro NOOP {
	v0 := v0
}

: filler_label # doesn't do anything
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
	NOOP
return

: main
	 filler_label
	 v1 := EXAMPLE_CONSTANT
return

const isn't a reserved keyword; :const is. The name "const" is an entirely valid label.

It would be a large breaking change to reserve all suffixes of reserved keywords, and I don't think it would do much to prevent mistakes.

Good point on backward compatibility. Thank you, I think your fixes close the scope of the original ticket. I'll think about the constant issues a little more and file another ticket proposing a fix that doesn't break compatibility with unusual label names.