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.

Sign up for GitHub

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

checkModuleName uses non-canonicalized path name #6547

Closed
wants to merge 1 commit into from
Closed

checkModuleName uses non-canonicalized path name #6547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions compiler/importer.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ proc getModuleName*(n: PNode): string =
result = ""

proc checkModuleName*(n: PNode; doLocalError=true): int32 =
# This returns the full canonical path for a given module import
# This returns the path for a given module import
let modulename = n.getModuleName
let fullPath = findModule(modulename, n.info.toFullPath)
if fullPath.len == 0:
let path = findModule(modulename, n.info.toPath)
if path.len == 0:
if doLocalError:
localError(n.info, errCannotOpenFile, modulename)
result = InvalidFileIDX
else:
result = fullPath.fileInfoIdx
result = path.fileInfoIdx

proc importPureEnumField*(c: PContext; s: PSym) =
var check = strTableGet(c.importTable.symbols, s.name)
Expand Down
19 changes: 14 additions & 5 deletions compiler/msgs.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ type
TFileInfo* = object
fullPath: string # This is a canonical full filesystem path
projPath*: string # This is relative to the project's root
path*: string # The original path when it was first encountered
Copy link
Member

Choose a reason for hiding this comment

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

Ugh no, not yet another path thing in here, patch shortName instead perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is getting more complicated now I have thought about it. If the nim compiler were to treat symlinks as normal files, for one unique fullPath (canonicalized as it is now) could correspond to different modules from different symlinks. The information inside TFileInfo does not seem to be enough. I'll try to make a more elaborate test case with symlinks to check.

Copy link
Member

@Araq Araq Oct 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We don't really support symlinks, they are a clusterfuck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If only we could all run plan9.

Copy link
Member

Choose a reason for hiding this comment

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

Completely besides the point, Windows doesn't really support symlinks anyway so your codebase will never work with one of the most widespread OSes out there, whether you like it or not. On Unix we can error on symlink'ed files since they are ambiguous for nim's purpose as you noted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting symlinks wouldn't make nim break on any OS that lacks symlinks (only plan9 comes to mind).

We can already use nim to create symlinks on Windows, #6287.

shortName*: string # short name of the module
quotedName*: Rope # cached quoted short name for codegen
# purposes
Expand Down Expand Up @@ -566,7 +567,8 @@ proc makeCString*(s: string): Rope =
add(result, rope(res))


proc newFileInfo(fullPath, projPath: string): TFileInfo =
proc newFileInfo(path, fullPath, projPath: string): TFileInfo =
result.path = path
result.fullPath = fullPath
#shallow(result.fullPath)
result.projPath = projPath
Expand Down Expand Up @@ -606,8 +608,8 @@ proc fileInfoIdx*(filename: string; isKnownFile: var bool): int32 =
else:
isKnownFile = false
result = fileInfos.len.int32
fileInfos.add(newFileInfo(canon, if pseudoPath: filename
else: canon.shortenDir))
fileInfos.add(newFileInfo(filename, canon, if pseudoPath: filename
else: filename.shortenDir))
filenameToIndexTbl[canon] = result

proc fileInfoIdx*(filename: string): int32 =
Expand All @@ -622,10 +624,10 @@ proc newLineInfo*(fileInfoIdx: int32, line, col: int): TLineInfo =
proc newLineInfo*(filename: string, line, col: int): TLineInfo {.inline.} =
result = newLineInfo(filename.fileInfoIdx, line, col)

fileInfos.add(newFileInfo("", "command line"))
fileInfos.add(newFileInfo("", "", "command line"))
var gCmdLineInfo* = newLineInfo(int32(0), 1, 1)

fileInfos.add(newFileInfo("", "compilation artifact"))
fileInfos.add(newFileInfo("", "", "compilation artifact"))
var gCodegenLineInfo* = newLineInfo(int32(1), 1, 1)

proc raiseRecoverableError*(msg: string) {.noinline, noreturn.} =
Expand Down Expand Up @@ -713,6 +715,10 @@ proc toFullPath*(fileIdx: int32): string =
if fileIdx < 0: result = "???"
else: result = fileInfos[fileIdx].fullPath

proc toPath*(fileIdx: int32): string =
if fileIdx < 0: result = "???"
else: result = fileInfos[fileIdx].path

proc setDirtyFile*(fileIdx: int32; filename: string) =
assert fileIdx >= 0
fileInfos[fileIdx].dirtyFile = filename
Expand All @@ -731,6 +737,9 @@ template toFilename*(info: TLineInfo): string =
template toFullPath*(info: TLineInfo): string =
info.fileIndex.toFullPath

template toPath*(info: TLineInfo): string =
info.fileIndex.toPath

proc toMsgFilename*(info: TLineInfo): string =
if info.fileIndex < 0:
result = "???"
Expand Down