compileopts: improve error reporting of unsupported flags #1073

cebernardi · 2020-04-26T11:41:07Z

1062 - Called Options.Validate method in main

niaow · 2020-04-26T14:39:08Z

compileopts/options.go

+
+var (
+	validGCOptions        = []string{"none", "leaking", "extalloc", "conservative"}
+	validSchedulerOptions = []string{"tasks", "coroutines"}


"none" is also a valid scheduler option

@jaddr2line oh I see.. so I think I should also modify this line https://github.com/tinygo-org/tinygo/blob/master/main.go#L713 and this line: https://github.com/tinygo-org/tinygo/blob/master/compileopts/config.go#L132

can you please confirm?

Ah, yes those should be updated. You could add it to this PR: even though it's somewhat separate it's a small and obvious fix.

@jaddr2line @aykevl done!

compileopts/options.go

niaow · 2020-04-26T14:42:40Z

compileopts/options.go

+	// ErrSchedulerInvalidOption is an error raised if scheduler option is not valid
+	ErrSchedulerInvalidOption = fmt.Errorf(`invalid scheduler option: valid values are %s`,
+		strings.Join(validSchedulerOptions, ", "))
+	//ErrPrintSizeInvalidOption is an error raised if size option is not valid


You need a space before the first word in a doc comment.

compileopts/options.go

compileopts/options_test.go

niaow · 2020-04-26T15:00:10Z

main.go

 		Programmer:    *programmer,
 	}

+	err := options.Verify()


I am not sure whether this is the right place to put this, as we continue modifying the options after this.

What do you think @aykevl ?

I had this doubt as well, I put it here because it looked to me that after this line nothing was setting GC, Scheduler or PrintSize.. very curious to see what's you thoughts are..

Yes, I think it should be done when the options object is finished building and won't be modified anymore.

Looking down below I see that there is also a custom check for panicStrategy. I think it would make sense to move that one to the common Verify function too.

@aykevl I moved panicStrategy check under options as well, and removed it from main.go

@aykevl @jaddr2line I've also moved options.Validate call later in the code..
I'm still unsure about it, because a few lines later, in the "build" case of the switch, options.Target is modified.

Another option could be moving the option check into builder.NewConfig...
what do you guys think?

@aykevl @jaddr2line what do you think about moving the options.Verify block into builder.NewConfig?
The more I think about it, the more it looks to me the right place 🤔

niaow · 2020-04-26T15:01:46Z

The macos build failure appears to have been caused by a partial CircleCI outage, which has since been fixed.

…rated options list from the errors

Co-Authored-By: Jaden Weiss <[email protected]>

…ygo into 1062-flag-error-reporting

aykevl · 2020-05-11T14:37:24Z

I think the current Verify call is fine. You could make a NewOptions, but I think a NewX function is mainly useful for initializing private fields or when complicated logic is required. In this case the contents of Options is public, and is intended to be public. The struct fields are the public API.

I think this PR is fine as it is right now and could be squashed/merged. If I were to suggest any changes, it would be the following:

Do not export the errors as globals, in my view they are an implementation detail (the errors are intended to be shown directly to the user, and not intended to be interpreted by the API consume).
In fact, I think it's better to generate error values on the fly so that you can include the invalid flag. For example, if you use -panic=foo, you get error: invalid panic option foo: valid values are print, trap. This improvs the user experience.
With this, the current tests become a lot harder. Honestly I don't see the value in testing here. Testing is important in many parts of the code (especially in complicated algorithms), but in this case there are no complicated APIs to test. The tests also get in the way of adding/removing options.

cebernardi · 2020-05-11T15:12:40Z

@aykevl probably I didn't explain myself, I apologies for this.
My question was:
Do we want to call options.Verify() in main.go, or do we prefer to call options.Verify() as a first thing in builder.NewConfig method?

builder.NewConfig takes an Option parameter, so I thought it was a good spot.

Regarding your other suggestions, no problem, I'll change the error generation to include the offending value 👍

cebernardi · 2020-05-16T11:02:13Z

@aykevl I've unexported the errors (actually, I got rid of them at all) and included the incorrect option value in the message.

aykevl

Looks good to me, thank you for the updates!

aykevl · 2020-05-16T21:33:55Z

Oops I didn't pay attention and I accidentally merged this into master. It should be fixed now.

cebernardi added 4 commits April 26, 2020 12:29

1062 - Added Validate method to Options struct

be9e804

1062 - Called Options.Validate method in main

fixed rebase disaster

e85d31c

fix task option

bdd53df

fix backward compatibility for go1.12 and 1.11

2e604f6

niaow requested changes Apr 26, 2020

View reviewed changes

niaow changed the title ~~1062 flag error reporting~~ compileopts: improve error reporting of unsupported flags Apr 26, 2020

cebernardi and others added 8 commits April 26, 2020 16:33

added periods to the documentation, fixed space in the comments, sepa…

27da1cc

…rated options list from the errors

changed raised to returned to better adhere to go semantic

7d50725

removed duplicated tests

5a60cd1

Update compileopts/options_test.go

2b963e0

Co-Authored-By: Jaden Weiss <[email protected]>

added periods at the end of the sentences

da72da3

Update compileopts/options_test.go

9c1f171

Co-Authored-By: Jaden Weiss <[email protected]>

changed test names to something more succing

49dac08

Merge branch '1062-flag-error-reporting' of github.com:cebernardi/tin…

41e3a7b

…ygo into 1062-flag-error-reporting

cebernardi requested a review from niaow April 26, 2020 16:11

cebernardi added 4 commits April 26, 2020 18:00

fixed grammar in comments

688e741

added 'none' as valid scheduler option

0e30e81

moved custom panic validation logic under options

a91f5b6

moved validation logic

2c07a2c

cebernardi requested a review from aykevl May 1, 2020 21:36

added incorrect option values to error messages

8efe5d1

changed invalid to incorrect for uniformity

1430c6c

aykevl approved these changes May 16, 2020

View reviewed changes

aykevl merged commit aff260f into tinygo-org:master May 16, 2020

niaow added this to the v0.14 milestone Jun 27, 2020

compileopts: improve error reporting of unsupported flags #1073

compileopts: improve error reporting of unsupported flags #1073

Uh oh!

Conversation

cebernardi commented Apr 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cebernardi May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niaow commented Apr 26, 2020

Uh oh!

aykevl commented May 11, 2020

Uh oh!

cebernardi commented May 11, 2020

Uh oh!

cebernardi commented May 16, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

aykevl commented May 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cebernardi May 10, 2020 •

edited

Loading