Fix #153 Scope FROM variables correctly

Variables that are used in a FROM instruction can only interact with the initial set of ARG instructions at the top before the first FROM instruction. ENV variables that are declared at the top of a Dockerfile cannot be processed by the Docker builder and should not be connected with any FROM instruction that refers to such a variable when processing a textDocument/definition, textDocument/hover, textDocument/documentHighlight, or textDocument/rename request. Signed-off-by: Remy Suen <[email protected]>
rcjsuen · Aug 28, 2017 · f4c186f · f4c186f
1 parent 62f2232
commit f4c186f
Show file tree

Hide file tree

Showing 10 changed files with 799 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@ All notable changes to this project will be documented in this file.
 - do not suggest duplicated build stage names as completion items ([#156](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/156))
 - only suggest build stages that come after the current COPY line ([#158](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/158))
 - restrict operations on ARG and ENV variables to a build stage ([#163](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/163))
+- make FROM variables only interact with the initial set of ARG instructions ([#153](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/153))
 
 ## [0.0.6] - 2017-08-12
 ### Added

diff --git a/src/dockerDefinition.ts b/src/dockerDefinition.ts
@@ -8,7 +8,7 @@ import { TextDocument, Position, Range, Location } from 'vscode-languageserver';
 import { Util } from './docker';
 import { Dockerfile } from './parser/dockerfile';
 import { DockerfileParser } from './parser/dockerfileParser';
-import { Image } from './parser/image';
+import { ImageTemplate } from './parser/imageTemplate';
 import { Property } from './parser/property';
 import { Arg } from './parser/instructions/arg';
 import { Env } from './parser/instructions/env';
@@ -35,7 +35,7 @@ export class DockerDefinition {
 		return null;
 	}
 
-	public static computeVariableDefinition(image: Image, position: Position): Property {
+	private static computeVariableDefinition(image: ImageTemplate, position: Position): Property {
 		let variableName = null;
 		for (let arg of image.getARGs()) {
 			let property = arg.getProperty();
@@ -92,9 +92,26 @@ export class DockerDefinition {
 		return null;
 	}
 
-	private computeVariableDefinition(uri: string, dockerfile: Dockerfile, position: Position): Location | null {
+	public static findDefinition(dockerfile: Dockerfile, position: Position): Property {
+		for (const from of dockerfile.getFROMs()) {
+			for (const variable of from.getVariables()) {
+				if (Util.isInsideRange(position, variable.getNameRange())) {
+					for (const arg of dockerfile.getInitialARGs()) {
+						const property = arg.getProperty();
+						if (property && property.getName() === variable.getName()) {
+							return property;
+						}
+					}
+					return null;
+				}
+			}
+		}
 		let image = dockerfile.getContainingImage(position);
-		let property = DockerDefinition.computeVariableDefinition(image, position);
+		return DockerDefinition.computeVariableDefinition(image, position);
+	}
+
+	private computeVariableDefinition(uri: string, dockerfile: Dockerfile, position: Position): Location | null {
+		const property = DockerDefinition.findDefinition(dockerfile, position);
 		return property ? Location.create(uri, property.getNameRange()) : null;
 	}
 

diff --git a/src/dockerHighlight.ts b/src/dockerHighlight.ts
@@ -8,6 +8,7 @@ import {
 	TextDocument, Position, DocumentHighlight, DocumentHighlightKind
 } from 'vscode-languageserver';
 import { DockerfileParser } from './parser/dockerfileParser';
+import { From } from './parser/instructions/from';
 import { DockerDefinition } from './dockerDefinition';
 import { Util } from './docker';
 
@@ -18,7 +19,7 @@ export class DockerHighlight {
 		let dockerfile = parser.parse(document);
 		let provider = new DockerDefinition();
 		let location = provider.computeDefinition(document, position);
-		let image = dockerfile.getContainingImage(position);
+		let image = location === null ? dockerfile.getContainingImage(position) : dockerfile.getContainingImage(location.range.start);
 		let highlights = [];
 		if (location === null) {
 			for (let instruction of dockerfile.getCOPYs()) {
@@ -36,15 +37,33 @@ export class DockerHighlight {
 				}
 			}
 
+			for (const from of dockerfile.getFROMs()) {
+				for (const variable of from.getVariables()) {
+					if (Util.isInsideRange(position, variable.getNameRange())) {
+						const name = variable.getName();
+						for (const loopFrom of dockerfile.getFROMs()) {
+							for (const fromVariable of loopFrom.getVariables()) {
+								if (fromVariable.getName() === name) {
+									highlights.push(DocumentHighlight.create(fromVariable.getNameRange(), DocumentHighlightKind.Read));
+								}
+							}
+						}
+						return highlights;
+					}
+				}
+			}
+
 			for (let instruction of image.getInstructions()) {
 				for (let variable of instruction.getVariables()) {
 					if (Util.isInsideRange(position, variable.getNameRange())) {
 						let name = variable.getName();
 
 						for (let instruction of image.getInstructions()) {
-							for (let variable of instruction.getVariables()) {
-								if (variable.getName() === name) {
-									highlights.push(DocumentHighlight.create(variable.getNameRange(), DocumentHighlightKind.Read));
+							if (!(instruction instanceof From)) {
+								for (let variable of instruction.getVariables()) {
+									if (variable.getName() === name) {
+										highlights.push(DocumentHighlight.create(variable.getNameRange(), DocumentHighlightKind.Read));
+									}
 								}
 							}
 						}
@@ -86,9 +105,25 @@ export class DockerHighlight {
 			}
 
 			for (let instruction of image.getInstructions()) {
-				for (let variable of instruction.getVariables()) {
-					if (variable.getName() === definition) {
-						highlights.push(DocumentHighlight.create(variable.getNameRange(), DocumentHighlightKind.Read));
+				// only highlight variables in non-FROM instructions
+				if (!(instruction instanceof From)) {
+					for (const variable of instruction.getVariables()) {
+						if (variable.getName() === definition) {
+							highlights.push(DocumentHighlight.create(variable.getNameRange(), DocumentHighlightKind.Read));
+						}
+					}
+				}
+			}
+
+			for (const arg of dockerfile.getInitialARGs()) {
+				const property = arg.getProperty();
+				if (property && Util.rangeEquals(property.getNameRange(), location.range)) {
+					for (const from of dockerfile.getFROMs()) {
+						for (const variable of from.getVariables()) {
+							if (variable.getName() === definition) {
+								highlights.push(DocumentHighlight.create(variable.getNameRange(), DocumentHighlightKind.Read));
+							}
+						}
 					}
 				}
 			}

diff --git a/src/dockerHover.ts b/src/dockerHover.ts
@@ -99,7 +99,7 @@ export class DockerHover {
 			}
 		}
 
-		let property = DockerDefinition.computeVariableDefinition(image, textDocumentPosition.position);
+		let property = DockerDefinition.findDefinition(dockerfile, textDocumentPosition.position);
 		if (property && property.getValue() !== null) {
 			return { contents: property.getValue() };
 		}

diff --git a/src/parser/dockerfile.ts b/src/parser/dockerfile.ts
@@ -6,15 +6,16 @@
 
 import { Position } from 'vscode-languageserver';
 import { Directive } from './directive';
-import { Image } from './image';
+import { ImageTemplate } from './imageTemplate';
 import { Instruction } from './instruction';
+import { Arg } from './instructions/arg';
 import { DIRECTIVE_ESCAPE } from '../docker';
 
-export class Dockerfile extends Image {
+export class Dockerfile extends ImageTemplate {
 
-	private readonly initialInstructions: Instruction[] = [];
-	private readonly buildStages: Image[] = [];
-	private currentBuildStage: Image;
+	private readonly initialInstructions = new ImageTemplate();
+	private readonly buildStages: ImageTemplate[] = [];
+	private currentBuildStage: ImageTemplate;
 	private directive: Directive | null = null;
 
 	/**
@@ -32,22 +33,26 @@ export class Dockerfile extends Image {
 		return '\\';
 	}
 
-	public getContainingImage(position: Position): Image {
+	public getInitialARGs(): Arg[] {
+		return this.initialInstructions.getARGs();
+	}
+
+	public getContainingImage(position: Position): ImageTemplate {
 		for (let buildStage of this.buildStages) {
 			if (buildStage.contains(position)) {
 				return buildStage;
 			}
 		}
-		return this;
+		return this.initialInstructions;
 	}
 
 	public addInstruction(instruction: Instruction): void {
 		if (instruction.getKeyword() === "FROM") {
-			this.currentBuildStage = new Image();
+			this.currentBuildStage = new ImageTemplate();
 			this.buildStages.push(this.currentBuildStage);
 			this.foundFrom = true;
 		} else if (!this.foundFrom) {
-			this.initialInstructions.push(instruction);
+			this.initialInstructions.addInstruction(instruction);
 		}
 
 		if (this.foundFrom) {

diff --git a/src/parser/image.ts → src/parser/imageTemplate.ts b/src/parser/image.ts → src/parser/imageTemplate.ts
@@ -17,7 +17,7 @@ import { Healthcheck } from './instructions/healthcheck';
 import { Onbuild } from './instructions/onbuild';
 import { Util } from '../docker';
 
-export class Image {
+export class ImageTemplate {
 
 	private readonly comments: Comment[] = [];
 	private readonly instructions: Instruction[] = [];

diff --git a/test/dockerDefinition.test.ts b/test/dockerDefinition.test.ts
@@ -17,6 +17,10 @@ function computeDefinition(document: TextDocument, position: Position): Location
 	return provider.computeDefinition(document, position);
 }
 
+function findDefinition(document: TextDocument, line: number, character: number): Location {
+	return provider.computeDefinition(document, Position.create(line, character));
+}
+
 function assertLocation(location: Location, uri: string, startLine: number, startCharacter: number, endLine: number, endCharacter: number) {
 	assert.equal(location.uri, uri);
 	assert.equal(location.range.start.line, startLine);
@@ -1124,4 +1128,150 @@ describe("Dockerfile Document Definition tests", function() {
 			});
 		});
 	});
+
+	describe("before FROM", function() {
+		describe("ARG", function() {
+			it("FROM lookup", function() {
+				let document = createDocument("ARG image=alpine\nFROM $image");
+				let location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+
+				document = createDocument("ARG image=alpine\nFROM $image\nFROM $image");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 2, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+			});
+
+			it("reused variable name", function() {
+				let document = createDocument("ARG image=alpine\nFROM $image\nARG image=alpine2");
+				let location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 2, 6);
+				assertLocation(location, document.uri, 2, 4, 2, 9);
+
+				document = createDocument("ARG image=alpine\nFROM $image\nARG image=alpine2\nFROM $image");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 2, 6);
+				assertLocation(location, document.uri, 2, 4, 2, 9);
+				location = findDefinition(document, 3, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+
+				document = createDocument("ARG image=alpine\nFROM $image\nFROM $image\nARG image=alpine2");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 2, 8);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 3, 6);
+				assertLocation(location, document.uri, 3, 4, 3, 9);
+			});
+
+			it("scoped", function() {
+				let document = createDocument("ARG image=alpine\nFROM alpine\nRUN echo $image");
+				let location = findDefinition(document, 2, 12);
+				assert.equal(location, null);
+			});
+
+			it("non-existent variable", function() {
+				let document = createDocument("FROM $image\nARG image");
+				let location = findDefinition(document, 0, 8);
+				assert.equal(location, null);
+
+				document = createDocument("ARG\nFROM $image");
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+
+				document = createDocument("ARG image=alpine\nFROM $image2\nARG image2=alpine2");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 2, 6);
+				assertLocation(location, document.uri, 2, 4, 2, 10);
+			});
+		});
+
+		describe("ENV", function() {
+			it("FROM lookup", function() {
+				let document = createDocument("ENV image=alpine\nFROM $image");
+				let location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+
+				document = createDocument("ENV image=alpine\nFROM $image\nFROM $image");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 2, 8);
+				assert.equal(location, null);
+			});
+
+			it("reused variable name", function() {
+				let document = createDocument("ENV image=alpine\nFROM $image\nENV image=alpine2");
+				let location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 2, 6);
+				assertLocation(location, document.uri, 2, 4, 2, 9);
+
+				document = createDocument("ENV image=alpine\nFROM $image\nENV image=alpine2\nFROM $image");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 2, 6);
+				assertLocation(location, document.uri, 2, 4, 2, 9);
+				location = findDefinition(document, 3, 8);
+				assert.equal(location, null);
+
+				document = createDocument("ENV image=alpine\nFROM $image\nFROM $image\nENV image=alpine2");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 2, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 3, 6);
+				assertLocation(location, document.uri, 3, 4, 3, 9);
+			});
+
+			it("scoped", function() {
+				let document = createDocument("ENV image=alpine\nFROM alpine\nRUN echo $image");
+				let location = findDefinition(document, 2, 12);
+				assert.equal(location, null);
+			});
+
+			it("non-existent variable", function() {
+				let document = createDocument("FROM $image\nENV image");
+				let location = findDefinition(document, 0, 8);
+				assert.equal(location, null);
+
+				document = createDocument("ENV\nFROM $image");
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+
+				document = createDocument("ENV image=alpine\nFROM $image2\nENV image2=alpine2");
+				location = findDefinition(document, 0, 6);
+				assertLocation(location, document.uri, 0, 4, 0, 9);
+				location = findDefinition(document, 1, 8);
+				assert.equal(location, null);
+				location = findDefinition(document, 2, 6);
+				assertLocation(location, document.uri, 2, 4, 2, 10);
+			});
+		});
+	});
 });