Skip to content

error reporting includes columns #2163

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

Merged
merged 17 commits into from
Sep 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions mypy/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ def __init__(self, data_dir: str,
version_id: str) -> None:
self.start_time = time.time()
self.data_dir = data_dir
self.errors = Errors(options.suppress_error_context)
self.errors = Errors(options.suppress_error_context, options.show_column_numbers)
self.errors.set_ignore_prefix(ignore_prefix)
self.lib_path = tuple(lib_path)
self.source_set = source_set
Expand Down Expand Up @@ -454,15 +454,15 @@ def module_not_found(self, path: str, line: int, id: str) -> None:
if ((self.options.python_version[0] == 2 and moduleinfo.is_py2_std_lib_module(id)) or
(self.options.python_version[0] >= 3 and moduleinfo.is_py3_std_lib_module(id))):
self.errors.report(
line, "No library stub file for standard library module '{}'".format(id))
self.errors.report(line, stub_msg, severity='note', only_once=True)
line, 0, "No library stub file for standard library module '{}'".format(id))
self.errors.report(line, 0, stub_msg, severity='note', only_once=True)
elif moduleinfo.is_third_party_module(id):
self.errors.report(line, "No library stub file for module '{}'".format(id))
self.errors.report(line, stub_msg, severity='note', only_once=True)
self.errors.report(line, 0, "No library stub file for module '{}'".format(id))
self.errors.report(line, 0, stub_msg, severity='note', only_once=True)
else:
self.errors.report(line, "Cannot find module named '{}'".format(id))
self.errors.report(line, '(Perhaps setting MYPYPATH '
'or using the "--silent-imports" flag would help)',
self.errors.report(line, 0, "Cannot find module named '{}'".format(id))
self.errors.report(line, 0, '(Perhaps setting MYPYPATH '
'or using the "--silent-imports" flag would help)',
severity='note', only_once=True)

def report_file(self, file: MypyFile) -> None:
Expand Down Expand Up @@ -1190,11 +1190,11 @@ def skipping_ancestor(self, id: str, path: str, ancestor_for: 'State') -> None:
manager = self.manager
manager.errors.set_import_context([])
manager.errors.set_file(ancestor_for.xpath)
manager.errors.report(-1, "Ancestor package '%s' silently ignored" % (id,),
manager.errors.report(-1, -1, "Ancestor package '%s' silently ignored" % (id,),
severity='note', only_once=True)
manager.errors.report(-1, "(Using --silent-imports, submodule passed on command line)",
manager.errors.report(-1, -1, "(Using --silent-imports, submodule passed on command line)",
severity='note', only_once=True)
manager.errors.report(-1, "(This note brought to you by --almost-silent)",
manager.errors.report(-1, -1, "(This note brought to you by --almost-silent)",
severity='note', only_once=True)

def skipping_module(self, id: str, path: str) -> None:
Expand All @@ -1204,11 +1204,13 @@ def skipping_module(self, id: str, path: str) -> None:
manager.errors.set_import_context(self.caller_state.import_context)
manager.errors.set_file(self.caller_state.xpath)
line = self.caller_line
manager.errors.report(line, "Import of '%s' silently ignored" % (id,),
manager.errors.report(line, 0,
"Import of '%s' silently ignored" % (id,),
severity='note')
manager.errors.report(line, "(Using --silent-imports, module not passed on command line)",
manager.errors.report(line, 0,
"(Using --silent-imports, module not passed on command line)",
severity='note', only_once=True)
manager.errors.report(line, "(This note courtesy of --almost-silent)",
manager.errors.report(line, 0, "(This note courtesy of --almost-silent)",
severity='note', only_once=True)
manager.errors.set_import_context(save_import_context)

Expand Down Expand Up @@ -1378,7 +1380,8 @@ def parse_file(self) -> None:
if id == '':
# Must be from a relative import.
manager.errors.set_file(self.xpath)
manager.errors.report(line, "No parent module -- cannot perform relative import",
manager.errors.report(line, 0,
"No parent module -- cannot perform relative import",
blocker=True)
continue
if id not in dep_line_map:
Expand Down Expand Up @@ -1492,7 +1495,7 @@ def load_graph(sources: List[BuildSource], manager: BuildManager) -> Graph:
continue
if st.id in graph:
manager.errors.set_file(st.xpath)
manager.errors.report(-1, "Duplicate module named '%s'" % st.id)
manager.errors.report(-1, -1, "Duplicate module named '%s'" % st.id)
manager.errors.raise_error()
graph[st.id] = st
new.append(st)
Expand Down
66 changes: 40 additions & 26 deletions mypy/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ class ErrorInfo:
# The line number related to this error within file.
line = 0 # -1 if unknown

# The column number related to this error with file.
column = 0 # -1 if unknown

# Either 'error' or 'note'.
severity = ''

Expand All @@ -42,13 +45,14 @@ class ErrorInfo:
only_once = False

def __init__(self, import_ctx: List[Tuple[str, int]], file: str, typ: str,
function_or_member: str, line: int, severity: str, message: str,
blocker: bool, only_once: bool) -> None:
function_or_member: str, line: int, column: int, severity: str,
message: str, blocker: bool, only_once: bool) -> None:
self.import_ctx = import_ctx
self.file = file
self.type = typ
self.function_or_member = function_or_member
self.line = line
self.column = column
self.severity = severity
self.message = message
self.blocker = blocker
Expand Down Expand Up @@ -92,7 +96,11 @@ class Errors:
# Set to True to suppress "In function "foo":" messages.
suppress_error_context = False # type: bool

def __init__(self, suppress_error_context: bool = False) -> None:
# Set to True to show column numbers in error messages
show_column_numbers = False # type: bool

def __init__(self, suppress_error_context: bool = False,
show_column_numbers: bool = False) -> None:
self.error_info = []
self.import_ctx = []
self.type_name = [None]
Expand All @@ -101,9 +109,10 @@ def __init__(self, suppress_error_context: bool = False) -> None:
self.used_ignored_lines = defaultdict(set)
self.only_once_messages = set()
self.suppress_error_context = suppress_error_context
self.show_column_numbers = show_column_numbers

def copy(self) -> 'Errors':
new = Errors(self.suppress_error_context)
new = Errors(self.suppress_error_context, self.show_column_numbers)
new.file = self.file
new.import_ctx = self.import_ctx[:]
new.type_name = self.type_name[:]
Expand Down Expand Up @@ -169,7 +178,7 @@ def set_import_context(self, ctx: List[Tuple[str, int]]) -> None:
"""Replace the entire import context with a new value."""
self.import_ctx = ctx[:]

def report(self, line: int, message: str, blocker: bool = False,
def report(self, line: int, column: int, message: str, blocker: bool = False,
severity: str = 'error', file: str = None, only_once: bool = False) -> None:
"""Report message at the given line using the current error context.

Expand All @@ -187,7 +196,7 @@ def report(self, line: int, message: str, blocker: bool = False,
if file is None:
file = self.file
info = ErrorInfo(self.import_context(), file, type,
self.function_or_member[-1], line, severity, message,
self.function_or_member[-1], line, column, severity, message,
blocker, only_once)
self.add_error_info(info)

Expand All @@ -210,7 +219,7 @@ def generate_unused_ignore_notes(self) -> None:
for line in ignored_lines - self.used_ignored_lines[file]:
# Don't use report since add_error_info will ignore the error!
info = ErrorInfo(self.import_context(), file, None, None,
line, 'note', "unused 'type: ignore' comment",
line, -1, 'note', "unused 'type: ignore' comment",
False, False)
self.error_info.append(info)

Expand Down Expand Up @@ -245,10 +254,13 @@ def messages(self) -> List[str]:
a = [] # type: List[str]
errors = self.render_messages(self.sort_messages(self.error_info))
errors = self.remove_duplicates(errors)
for file, line, severity, message in errors:
for file, line, column, severity, message in errors:
s = ''
if file is not None:
if line is not None and line >= 0:
if self.show_column_numbers and line is not None and line >= 0 \
and column is not None and column >= 0:
srcloc = '{}:{}:{}'.format(file, line, column)
elif line is not None and line >= 0:
srcloc = '{}:{}'.format(file, line)
else:
srcloc = file
Expand All @@ -258,16 +270,17 @@ def messages(self) -> List[str]:
a.append(s)
return a

def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[str, int,
def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[str, int, int,
str, str]]:
"""Translate the messages into a sequence of tuples.

Each tuple is of form (path, line, message. The rendered
Each tuple is of form (path, line, col, message. The rendered
sequence includes information about error contexts. The path
item may be None. If the line item is negative, the line
number is not defined for the tuple.
"""
result = [] # type: List[Tuple[str, int, str, str]] # (path, line, severity, message)
result = [] # type: List[Tuple[str, int, int, str, str]]
# (path, line, column, severity, message)

prev_import_context = [] # type: List[Tuple[str, int]]
prev_function_or_member = None # type: str
Expand All @@ -290,7 +303,7 @@ def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[str, int,
# Remove prefix to ignore from path (if present) to
# simplify path.
path = remove_path_prefix(path, self.ignore_prefix)
result.append((None, -1, 'note', fmt.format(path, line)))
result.append((None, -1, -1, 'note', fmt.format(path, line)))
i -= 1

file = self.simplify_path(e.file)
Expand All @@ -302,27 +315,27 @@ def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[str, int,
e.type != prev_type):
if e.function_or_member is None:
if e.type is None:
result.append((file, -1, 'note', 'At top level:'))
result.append((file, -1, -1, 'note', 'At top level:'))
else:
result.append((file, -1, 'note', 'In class "{}":'.format(
result.append((file, -1, -1, 'note', 'In class "{}":'.format(
e.type)))
else:
if e.type is None:
result.append((file, -1, 'note',
result.append((file, -1, -1, 'note',
'In function "{}":'.format(
e.function_or_member)))
else:
result.append((file, -1, 'note',
result.append((file, -1, -1, 'note',
'In member "{}" of class "{}":'.format(
e.function_or_member, e.type)))
elif e.type != prev_type:
if e.type is None:
result.append((file, -1, 'note', 'At top level:'))
result.append((file, -1, -1, 'note', 'At top level:'))
else:
result.append((file, -1, 'note',
result.append((file, -1, -1, 'note',
'In class "{}":'.format(e.type)))

result.append((file, e.line, e.severity, e.message))
result.append((file, e.line, e.column, e.severity, e.message))

prev_import_context = e.import_ctx
prev_function_or_member = e.function_or_member
Expand All @@ -348,22 +361,23 @@ def sort_messages(self, errors: List[ErrorInfo]) -> List[ErrorInfo]:
i += 1
i += 1

# Sort the errors specific to a file according to line number.
a = sorted(errors[i0:i], key=lambda x: x.line)
# Sort the errors specific to a file according to line number and column.
a = sorted(errors[i0:i], key=lambda x: (x.line, x.column))
result.extend(a)
return result

def remove_duplicates(self, errors: List[Tuple[str, int, str, str]]
) -> List[Tuple[str, int, str, str]]:
def remove_duplicates(self, errors: List[Tuple[str, int, int, str, str]]
) -> List[Tuple[str, int, int, str, str]]:
"""Remove duplicates from a sorted error list."""
res = [] # type: List[Tuple[str, int, str, str]]
res = [] # type: List[Tuple[str, int, int, str, str]]
i = 0
while i < len(errors):
dup = False
j = i - 1
while (j >= 0 and errors[j][0] == errors[i][0] and
errors[j][1] == errors[i][1]):
if errors[j] == errors[i]:
if (errors[j][3] == errors[i][3] and
errors[j][4] == errors[i][4]): # ignore column
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't the 0th and first elements need to be compared too?

also, is ignoring the column necessary b/c of the TODO in TypeConverter#generic_visit in fastparse.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're taken care of because we're within the while loop.

Ignoring the column here, so that we only get the first instance of this particular error within the line. This logic could be tweaked, for sure. Maybe we actually do want to expose all instances of a given error. E.g.

  • invalid type at column 1
  • invalid type at column 5

dup = True
break
j -= 1
Expand Down
7 changes: 4 additions & 3 deletions mypy/expandtype.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,15 @@ def visit_erased_type(self, t: ErasedType) -> Type:

def visit_instance(self, t: Instance) -> Type:
args = self.expand_types(t.args)
return Instance(t.type, args, t.line)
return Instance(t.type, args, t.line, t.column)

def visit_type_var(self, t: TypeVarType) -> Type:
repl = self.variables.get(t.id, t)
if isinstance(repl, Instance):
inst = repl
# Return copy of instance with type erasure flag on.
return Instance(inst.type, inst.args, inst.line, True)
return Instance(inst.type, inst.args, line=inst.line,
column=inst.column, erased=True)
else:
return repl

Expand All @@ -93,7 +94,7 @@ def visit_tuple_type(self, t: TupleType) -> Type:
def visit_union_type(self, t: UnionType) -> Type:
# After substituting for type variables in t.items,
# some of the resulting types might be subtypes of others.
return UnionType.make_simplified_union(self.expand_types(t.items), t.line)
return UnionType.make_simplified_union(self.expand_types(t.items), t.line, t.column)

def visit_partial_type(self, t: PartialType) -> Type:
return t
Expand Down
17 changes: 10 additions & 7 deletions mypy/fastparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
except (SyntaxError, TypeCommentParseError) as e:
if errors:
errors.set_file('<input>' if fnam is None else fnam)
errors.report(e.lineno, e.msg)
errors.report(e.lineno, e.offset, e.msg)
else:
raise

Expand All @@ -84,8 +84,8 @@ def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
def parse_type_comment(type_comment: str, line: int) -> Type:
try:
typ = ast35.parse(type_comment, '<type_comment>', 'eval')
except SyntaxError:
raise TypeCommentParseError(TYPE_COMMENT_SYNTAX_ERROR, line)
except SyntaxError as e:
raise TypeCommentParseError(TYPE_COMMENT_SYNTAX_ERROR, line, e.offset)
else:
assert isinstance(typ, ast35.Expression)
return TypeConverter(line=line).visit(typ.body)
Expand All @@ -95,7 +95,7 @@ def with_line(f: Callable[['ASTConverter', T], U]) -> Callable[['ASTConverter',
@wraps(f)
def wrapper(self: 'ASTConverter', ast: T) -> U:
node = f(self, ast)
node.set_line(ast.lineno)
node.set_line(ast.lineno, ast.col_offset)
return node
return wrapper

Expand Down Expand Up @@ -260,7 +260,7 @@ def do_func_def(self, n: Union[ast35.FunctionDef, ast35.AsyncFunctionDef],
try:
func_type_ast = ast35.parse(n.type_comment, '<func_type>', 'func_type')
except SyntaxError:
raise TypeCommentParseError(TYPE_COMMENT_SYNTAX_ERROR, n.lineno)
raise TypeCommentParseError(TYPE_COMMENT_SYNTAX_ERROR, n.lineno, n.col_offset)
assert isinstance(func_type_ast, ast35.FunctionType)
# for ellipsis arg
if (len(func_type_ast.argtypes) == 1 and
Expand Down Expand Up @@ -600,6 +600,7 @@ def visit_UnaryOp(self, n: ast35.UnaryOp) -> Node:
def visit_Lambda(self, n: ast35.Lambda) -> Node:
body = ast35.Return(n.body)
body.lineno = n.lineno
body.col_offset = n.col_offset

return FuncExpr(self.transform_args(n.args, n.lineno),
self.as_block([body], n.lineno))
Expand Down Expand Up @@ -804,7 +805,8 @@ def visit_raw_str(self, s: str) -> Type:
return parse_type_comment(s.strip(), line=self.line)

def generic_visit(self, node: ast35.AST) -> None:
raise TypeCommentParseError(TYPE_COMMENT_AST_ERROR, self.line)
raise TypeCommentParseError(TYPE_COMMENT_AST_ERROR, self.line,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately I think we still need the getattr here... not all AST nodes come with column info, only the ones deriving from stmt, expr, etc...

(https://github.com/python/typeshed/blob/master/third_party/3/typed_ast/ast35.pyi)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine!

getattr(node, 'col_offset', -1))

def visit_NoneType(self, n: Any) -> Type:
return None
Expand Down Expand Up @@ -860,6 +862,7 @@ def visit_List(self, n: ast35.List) -> Type:


class TypeCommentParseError(Exception):
def __init__(self, msg: str, lineno: int) -> None:
def __init__(self, msg: str, lineno: int, offset: int) -> None:
self.msg = msg
self.lineno = lineno
self.offset = offset
Loading