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

Feature/2280 track all line numbers #2357

Merged
merged 19 commits into from Jul 31, 2017

Conversation

Projects
None yet
3 participants
@mitzimorris
Member

mitzimorris commented Jul 18, 2017

Submisison Checklist

  • Run unit tests: ./runTests.py src/test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary

modified Stan compiler so that line numbers of variable declarations are available for runtime error reporting.

On the parser side:

  • stan/lang/ast/node/var_decl.hpp: added fields begin_line and end_line
  • stan/lang/grammars/var_decls_grammar and semantic_actions record the raw positions from the Stan source file.

On the generator side:

  • generate variable declarations functions generate current_statement_begin__ statements before generating the declaration and initialization statements.
  • try and catch blocks used by stan/lang/rethrow_located.hpp now enclose variable declarations as well as the statements.
  • changing the position of the try and catch blocks relative to the generated code changes the relative indentation. the generator functions were inconsistent in how they did the indentation for the generated code. some functions hardcoded the level of indent using constants INDENT INDENT2 and INDENT3. other functions take an integer argument indent for the current level of indentation. as the latter is preferable, I changed the generator functions so that they take an indent argument instead of hardcoding the indentation level. however, the functions which generate the top-level code sections still use the constants. this required changing many of the generator functions and visitors.

Intended Effect

adding compound declare/define statements to the language created many situations for runtime errors, e.g., assignment to a container where there is a size mismatch on one of the dimensions. this should make it easier to debug runtime errors.

How to Verify

added new directory runtime_errors to src/test/test-models/good. these models should compile. running them via command stan interface with command line arguments sample algorithm=fixed_param should generate an informative error message w/ line numbers.

note: this should be automated. how?

Side Effects

generated c++ model code may have spurious current_statement_begin__ statements because some of the generator's visgen functions don't need to do anything, e.g. for scalar variables.

Documentation

NA

Reviewer Suggestions

Bob Carpenter, Daniel Lee, Sean Talts

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Mitzi Morris

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

mitzimorris added some commits Jul 18, 2017

@bob-carpenter

Basically, this is OK as is. There are some cosmetic and code replication issues that'd be nice to iron out, but I'll just approve this now and leave it up to you whether or not the extra effort's worth it.

pull requests---it was way harder to review the new line number stuff in the context of all the indentation changing.

o_ << "vals_i__ = context__.vals_i(\"" << x.name_ << "\");" << EOL;
generate_indent(indent_, o_);
o_ << "pos__ = 0;" << EOL;
size_t indentation = indent_;

This comment has been minimized.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Is there a reason these are size_t in some places and std::size_t in others? I think the unqualified size_t get brought in with a lot of includes (like vector from the standard lib).

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Is there a reason these are size_t in some places and std::size_t in others? I think the unqualified size_t get brought in with a lot of includes (like vector from the standard lib).

This comment has been minimized.

@mitzimorris

mitzimorris Jul 27, 2017

Member

std::size_t is used only in the following files:

> find src/stan/lang -name "*.[ch]pp" -exec grep -l "std::size_t" {} \;
src/stan/lang/ast/expr_type.hpp
src/stan/lang/ast/expr_type_def.hpp
src/stan/lang/ast/fun/infer_type_indexing.hpp
src/stan/lang/ast/fun/total_dims.hpp
src/stan/lang/ast/node/statement.hpp
src/stan/lang/ast/node/var_decl.hpp
src/stan/lang/ast/node/variable.hpp
src/stan/lang/ast/variable_map.hpp

the generator code uses unqualified size_t everywhere.

@mitzimorris

mitzimorris Jul 27, 2017

Member

std::size_t is used only in the following files:

> find src/stan/lang -name "*.[ch]pp" -exec grep -l "std::size_t" {} \;
src/stan/lang/ast/expr_type.hpp
src/stan/lang/ast/expr_type_def.hpp
src/stan/lang/ast/fun/infer_type_indexing.hpp
src/stan/lang/ast/fun/total_dims.hpp
src/stan/lang/ast/node/statement.hpp
src/stan/lang/ast/node/var_decl.hpp
src/stan/lang/ast/node/variable.hpp
src/stan/lang/ast/variable_map.hpp

the generator code uses unqualified size_t everywhere.

generate_indent(indent_, o_);
o_ << "pos__ = 0;" << EOL;
generate_indent(indent_, o_);
o_ << "size_t " << x.name_ << "_i_vec_lim__ = ";

This comment has been minimized.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

same question for the generated code

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

same question for the generated code

Show outdated Hide outdated src/stan/lang/grammars/var_decls_grammar_def.hpp
Show outdated Hide outdated src/stan/lang/rethrow_located.hpp
Show outdated Hide outdated src/stan/lang/grammars/semantic_actions_def.cpp
generate_try(indent, o);
generate_statement(s, indent+1, o, include_sampling, is_var_context,
is_fun_return);
generate_catch_throw_located(indent, o);

This comment has been minimized.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Is the try/catch being generated at some higher level?

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Is the try/catch being generated at some higher level?

This comment has been minimized.

@mitzimorris
@mitzimorris
Show outdated Hide outdated src/stan/lang/generator/generate_log_prob.hpp
@@ -13,15 +15,23 @@ namespace stan {
/**
* Generate initializations for member variables by reading from
* constructor variable context.
* Generated initializations are preceeded by stmt updating global variable
* `current_statement_begin__` to src file line number where

This comment has been minimized.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Does this form of code markdown work in Doxygen?

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Does this form of code markdown work in Doxygen?

This comment has been minimized.

@mitzimorris
@mitzimorris
set_param_ranges_visgen vis(indent, o);
for (size_t i = 0; i < var_decls.size(); ++i) {
generate_indent(indent, o);
o << "current_statement_begin__ = " << var_decls[i].begin_line_ << ";"

This comment has been minimized.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

I think this generating line number thing should be pulled out into a function because you do it more than once. It can be templated if the arguments all have begin_line_ member variables.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

I think this generating line number thing should be pulled out into a function because you do it more than once. It can be templated if the arguments all have begin_line_ member variables.

This comment has been minimized.

@mitzimorris

mitzimorris Jul 27, 2017

Member

done.

@mitzimorris
@@ -53,7 +53,7 @@ namespace stan {
bool operator()(const reject_statement& st) const { return true; }
bool operator()(const no_op_statement& st) const { return true; }

This comment has been minimized.

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Nice fix!

@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Nice fix!

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Jul 25, 2017

Contributor

Heroic job, changing all the indentation, by the way. That was a massive number of changes and the new design's much better.

Contributor

bob-carpenter commented Jul 25, 2017

Heroic job, changing all the indentation, by the way. That was a massive number of changes and the new design's much better.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Jul 27, 2017

Contributor

@mitzimorris --- you declared this done, but did you push the commits? The last tests were run many days ago and we need to make sure they pass.

Contributor

bob-carpenter commented Jul 27, 2017

@mitzimorris --- you declared this done, but did you push the commits? The last tests were run many days ago and we need to make sure they pass.

mitzimorris added some commits Jul 27, 2017

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jul 28, 2017

Member

@seantalts - PR fails on Travis but tests pass on my machine - any insights would be much appreciated.

Member

mitzimorris commented Jul 28, 2017

@seantalts - PR fails on Travis but tests pass on my machine - any insights would be much appreciated.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Jul 28, 2017

Member

Hah, it seems like Travis just switched over to using Trusty by default? Luckily the config I made for C++11 for the math library uses Trusty already. I think maybe I need to port that to this repo now.

Member

seantalts commented Jul 28, 2017

Hah, it seems like Travis just switched over to using Trusty by default? Luckily the config I made for C++11 for the math library uses Trusty already. I think maybe I need to port that to this repo now.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Jul 28, 2017

Member

Actually since the makefiles etc are still in a state of flux I'm just going to fix this in a quick way with this pull request (for now): #2368

Member

seantalts commented Jul 28, 2017

Actually since the makefiles etc are still in a state of flux I'm just going to fix this in a quick way with this pull request (for now): #2368

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jul 28, 2017

Member

many thanks!

Member

mitzimorris commented Jul 28, 2017

many thanks!

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jul 30, 2017

Member

@seantalts - OK to close this?

Member

mitzimorris commented Jul 30, 2017

@seantalts - OK to close this?

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Jul 30, 2017

Contributor

@mitzimorris Did you mean merge it rather than close it?

Contributor

bob-carpenter commented Jul 30, 2017

@mitzimorris Did you mean merge it rather than close it?

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jul 30, 2017

Member

no, I meant close it - my understanding is that this feature was merged by this PR:
#2368

was it?
cheers,
Mitzi

Member

mitzimorris commented Jul 30, 2017

no, I meant close it - my understanding is that this feature was merged by this PR:
#2368

was it?
cheers,
Mitzi

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Jul 30, 2017

Member

Oh, that quick fix was only for the travis config. This PR is still separate. You should merge or rebase develop into this branch and push it back up and hopefully the travis tests will pass then.

Member

seantalts commented Jul 30, 2017

Oh, that quick fix was only for the travis config. This PR is still separate. You should merge or rebase develop into this branch and push it back up and hopefully the travis tests will pass then.

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jul 30, 2017

Member

OK, merged w/ develop and pushed. fingers crossed.

Member

mitzimorris commented Jul 30, 2017

OK, merged w/ develop and pushed. fingers crossed.

@mitzimorris mitzimorris merged commit b3ace84 into develop Jul 31, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@mitzimorris mitzimorris deleted the feature/2280-track-all-line-numbers branch Jul 31, 2017

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